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

Consolidate cryptography tests to System.Security.Cryptography #66338

Open
vcsjones opened this issue Mar 8, 2022 · 4 comments
Open

Consolidate cryptography tests to System.Security.Cryptography #66338

vcsjones opened this issue Mar 8, 2022 · 4 comments

Comments

@vcsjones
Copy link
Member

vcsjones commented Mar 8, 2022

With #55690 merged, the implementations of the S.S.Cryptography assemblies have been consolidated in to a single in-box assembly, where possible.

The unit tests for these are still in their separate projects however, such as S.S.C.Algorithms, S.S.C.Cng, etc.

This issue is to track consolidating them under System.Security.Cryptography. Note that this is not as simple as moving the files and adjusting namespaces. We made heavy use of partials to test different implementations of algorithms (internal vs public CNG for example). The partial approach won't work. The current solution is to refactor to a "driver" base type and test each implementation as a derived type.

@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Security untriaged New issue has not been triaged by the area owner labels Mar 8, 2022
@ghost
Copy link

ghost commented Mar 8, 2022

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

Issue Details

With #55690 merged, the implementations of the S.S.Cryptography assemblies have been consolidated in to a single in-box assembly, where possible.

The unit tests for these are still in their separate projects however, such as S.S.C.Algorithms, S.S.C.Cng, etc.

This issue is to track consolidating them under System.Security.Cryptography. Note that this is not as simple as moving the files and adjusting namespaces. We made heavy use of partials to test different implementations of algorithms (internal vs public CNG for example). The partial approach won't work. The current solution is to refactor to a "driver" base type and test each implementation as a derived type.

Author: vcsjones
Assignees: -
Labels:

area-System.Security, untriaged

Milestone: -

@bartonjs
Copy link
Member

bartonjs commented Mar 8, 2022

Huh, thought I'd opened this already. But, I hadn't. Thanks 😄.

https://github.com/bartonjs/runtime/commits/crypto_test_model has my first-draft thoughts here. They seemed to be fruitful, but certainly aren't a commitment.

@vcsjones
Copy link
Member Author

We could move X509Certificates pretty easily, if not for:

[assembly: ActiveIssue("https://github.com/dotnet/runtime/issues/57810", typeof(PlatformDetection), nameof(PlatformDetection.IsMonoRuntime), nameof(PlatformDetection.IsMariner))]

I don't think we want to disable all of S.S.Cryptography tests on Mariner. But do we actually need to disable X509Certificates on Mariner? According to #57810 (comment), that ActiveIssue didn't stop the Mariner crashes.

I think the given options are:

  1. Block moving X509Certificate tests until System.Security.Cryptography.X509Certificates.Tests crashing on Mariner OS #57810 is solved.
  2. Put ActiveIssue on every single X509Certificates class, and future classes.
  3. Move all of the tests anyway and remove the assembly-level ActiveIssue since prior comment indicated it did not help. If that is true, then what is being done for Mariner? Are tests running, or did we disable them some other way?

cc @ViktorHofer in case you have any insight on the X509Certificate tests in Mariner.

@bartonjs bartonjs removed the untriaged New issue has not been triaged by the area owner label Jul 6, 2022
@bartonjs bartonjs added this to the Future milestone Jul 6, 2022
@bartonjs
Copy link
Member

bartonjs commented Jul 6, 2022

Moving to the Future/Indefinite milestone because it's just code cleanup and our own pain, nothing impacting customers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants