Merge System.Security.Cryptography.Algorithms to System.Security.Cryptography #61543

bartonjs · 2021-11-13T01:28:59Z

Merge S.S.C.Algorithms into S.S.Cryptography as expected for Windows/Unix/Android/macOS/iOS/tvOS
Merge S.S.C.Algorithms into S.S.Cryptography with no PNSE generation for Browser
- This resulted in a lot of UnsupportedOSPlatform attributes being removed, and a lesser number being moved from the class to a factory method.
- There was also a small amount of plumbing work needed to handle all of the resulting platform compat warnings.
Merge S.S.C.Algorithms.Tests into S.S.Cryptography.Tests, renaming namespaces for files that moved, except the file is a partial to a file in Common\test

This move resulted in a net reduction of about 25 resource strings from a combination of the string already being present in the unified library, perhaps some strings that already had no references, and a few strings got abandoned (unified onto another existing string).

Three notable changes exist in the resource string moves:

Cryptography_MissingKey => Cryptography_FormatterMissingKey
- Added "Formatter" to the resource name because the message wasn't as generic as it sounded.
- Added an actionable component since it was already pretty darn specific.
- Before: No asymmetric key object has been associated with this formatter object.
- After: No asymmetric key object has been associated with this formatter object, assign one via the SetKey method.
Cryptography_MissingOID => Cryptography_FormatterMissingAlgorithm
- The resource key name didn't match the code, or the action a user needed to take.
- The resource value didn't match the code, or the action a user needed to take.
- Completely rewrote it to look like Cryptography_FormatterMissingKey
- Before: Required object identifier (OID) cannot be found.
- After: No hash algorithm has been associated with this formatter object, assign one via the SetHashAlgorithm method.
Cryptography_Prk_TooSmall
- Added "at least" because it read a little misleadingly as exact.
- Before: The pseudo-random key length must be {0} bytes.
- After: The pseudo-random key length must be at least {0} bytes.

The "Unix" files were generally renamed to "OpenSsl" in this change. The Android build still uses many of those files, copying them to an Android specific version is being left to a followup change.

The crypto tests definitely look a bit untidy at this point. The static classes and [Algorithm]Provider model will have to be redone before the Cng/Csp/OpenSsl libraries can be merged in. Since this change is largely about library consolidation, no test cleanup was done with this change.

ghost · 2021-11-13T01:29:08Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2021-11-13T01:29:13Z

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq, @GrabYourPitchforks
See info in area-owners.md if you want to be subscribed.

Issue Details

Merge S.S.C.Algorithms into S.S.Cryptography as expected for Windows/Unix/Android/macOS/iOS/tvOS
Merge S.S.C.Algorithms into S.S.Cryptography with no PNSE generation for Browser
- This resulted in a lot of UnsupportedOSPlatform attributes being removed, and a lesser number being moved from the class to a factory method.
- There was also a small amount of plumbing work needed to handle all of the resulting platform compat warnings.
Merge S.S.C.Algorithms.Tests into S.S.Cryptography.Tests, renaming namespaces for files that moved, except the file is a partial to a file in Common\test

This move resulted in a net reduction of about 25 resource strings from a combination of the string already being present in the unified library, perhaps some strings that already had no references, and a few strings got abandoned (unified onto another existing string).

Three notable changes exist in the resource string moves:

Cryptography_MissingKey => Cryptography_FormatterMissingKey
- Added "Formatter" to the resource name because the message wasn't as generic as it sounded.
- Added an actionable component since it was already pretty darn specific.
- Before: No asymmetric key object has been associated with this formatter object.
- After: No asymmetric key object has been associated with this formatter object, assign one via the SetKey method.
Cryptography_MissingOID => Cryptography_FormatterMissingAlgorithm
- The resource key name didn't match the code, or the action a user needed to take.
- The resource value didn't match the code, or the action a user needed to take.
- Completely rewrote it to look like Cryptography_FormatterMissingKey
- Before: Required object identifier (OID) cannot be found.
- After: No hash algorithm has been associated with this formatter object, assign one via the SetHashAlgorithm method.
Cryptography_Prk_TooSmall
- Added "at least" because it read a little misleadingly as exact.
- Before: The pseudo-random key length must be {0} bytes.
- After: The pseudo-random key length must be at least {0} bytes.

The "Unix" files were generally renamed to "OpenSsl" in this change. The Android build still uses many of those files, copying them to an Android specific version is being left to a followup change.

The crypto tests definitely look a bit untidy at this point. The static classes and [Algorithm]Provider model will have to be redone before the Cng/Csp/OpenSsl libraries can be merged in. Since this change is largely about library consolidation, no test cleanup was done with this change.

Author:	bartonjs
Assignees:	-
Labels:	`area-System.Security`, `new-api-needs-documentation`
Milestone:	-

bartonjs · 2021-11-13T01:39:53Z

@safern for the structural/infra changes, please

@GrabYourPitchforks / @stephentoub for the impact of bringing browser fully into the fold here. Specifically the second commit (440df74) (yeah, it'll all get squashed in the end)

safern · 2021-11-15T23:06:24Z

src/libraries/System.Security.Cryptography/src/System.Security.Cryptography.csproj

@@ -1,7 +1,9 @@
 <Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
+    <DefineConstants>$(DefineConstants);INTERNAL_ASYMMETRIC_IMPLEMENTATIONS</DefineConstants>


Since we are merging pretty much all of Crypto into one assembly, is this still needed?

It's still needed in this iteration, yeah. It'll go away between this one and the next. (That is, before the next library merges in; or as part of that)

safern

Infra changes look good. However CI is red due to some of these.

bartonjs · 2021-11-15T23:21:31Z

However CI is red due to some of these.

Yeah. I was verifying that I'd fixed them all today when the power went out. Sorta... slowed things down.

GrabYourPitchforks · 2021-11-17T19:37:25Z

Re: browser impact, I suspect we'd want to add an ILLink.Substitutions.xml file so that patterns like if (!Helpers.SupportsSymmetricEncryption) { /* ... */ } are pruned during build.

With that substitution file in place, I'd assume no measurable impact to non-browser platforms?

GrabYourPitchforks · 2021-11-17T19:43:58Z

src/libraries/Common/src/System/Security/Cryptography/PasswordBasedEncryption.cs

@@ -556,6 +580,12 @@ private static CryptographicException AlgorithmKdfRequiresChars(string algId)
        {
            string? algId = encryptionScheme.Algorithm;

+            if (!Helpers.HasSymmetricEncryption)


This (+ HasHMAC below) are becoming something of a common pattern. Would it make sense to refactor the "check + throw an appropriate exception" logic into its own helper method?

M...aybe? They're here because of the UnsupportedOS flow analyzer, to show it that there's no way for Browser to make it to the annotated-as-failing Creates. I don't know how much flexibility we have with what it understands.

bartonjs · 2021-11-17T19:59:44Z

I suspect we'd want to add an ILLink.Substitutions.xml file so that patterns like if (!Helpers.SupportsSymmetricEncryption) { /* ... */ } are pruned during build.

Well, except we're going to try getting them enabled this release. So these helpers will probably drop out :)

This assembly has been removed with dotnet/runtime#61543

[main] Update dependencies from dotnet/runtime - Merge branch 'main' of https://github.com/dotnet/sdk into darc-main-79a18e20-75f6-4bb7-8d8e-5ae13e5c66b0 - Update baselines to remove Cryptography.Algorithms. This assembly has been removed with dotnet/runtime#61543 - More baseline updates

bartonjs requested review from GrabYourPitchforks, stephentoub and safern November 13, 2021 01:28

ghost added area-System.Security new-api-needs-documentation labels Nov 13, 2021

bartonjs force-pushed the unified_crypto_algorithms branch from ffd8923 to abb6791 Compare November 13, 2021 01:38

safern reviewed Nov 15, 2021

View reviewed changes

safern approved these changes Nov 15, 2021

View reviewed changes

bartonjs added 6 commits November 16, 2021 13:12

Unify S.S.C.Algorithms to S.S.Cryptography, except Browser

6f5bfa8

Make Browser build work

cb2c701

Make asn.xml files show up in Solution Explorer

678b8ec

Complete the removal of S.S.C.Algorithms

3307078

Merge S.S.C.Algorithms.Tests to S.S.Cryptography.Tests

66da3b0

Fix build breaks in the allConfigurations build from Linux

c48fea3

bartonjs force-pushed the unified_crypto_algorithms branch from fc7cf75 to c48fea3 Compare November 16, 2021 23:09

Suppress tests that don't work on Browser

6ea20e3

bartonjs removed the new-api-needs-documentation label Nov 17, 2021

GrabYourPitchforks approved these changes Nov 17, 2021

View reviewed changes

bartonjs merged commit 899bf97 into dotnet:main Nov 17, 2021

vcsjones mentioned this pull request Nov 19, 2021

Let argument check run before aborting check. #61795

Closed

eerhardt mentioned this pull request Nov 30, 2021

[main] Update dependencies from dotnet/runtime dotnet/sdk#22668

Merged

eerhardt added a commit to dotnet/sdk that referenced this pull request Nov 30, 2021

Update baselines to remove Cryptography.Algorithms.

04b25e1

This assembly has been removed with dotnet/runtime#61543

ghost locked as resolved and limited conversation to collaborators Dec 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Merge System.Security.Cryptography.Algorithms to System.Security.Cryptography #61543

Merge System.Security.Cryptography.Algorithms to System.Security.Cryptography #61543

Uh oh!

bartonjs commented Nov 13, 2021

Uh oh!

ghost commented Nov 13, 2021

Uh oh!

ghost commented Nov 13, 2021

Uh oh!

bartonjs commented Nov 13, 2021

Uh oh!

safern Nov 15, 2021 •

edited

Loading

Uh oh!

bartonjs Nov 15, 2021 •

edited

Loading

Uh oh!

safern left a comment

Uh oh!

bartonjs commented Nov 15, 2021

Uh oh!

GrabYourPitchforks commented Nov 17, 2021

Uh oh!

GrabYourPitchforks Nov 17, 2021

Uh oh!

bartonjs Nov 17, 2021

Uh oh!

bartonjs commented Nov 17, 2021

Uh oh!

Uh oh!

Merge System.Security.Cryptography.Algorithms to System.Security.Cryptography #61543

Merge System.Security.Cryptography.Algorithms to System.Security.Cryptography #61543

Uh oh!

Conversation

bartonjs commented Nov 13, 2021

Uh oh!

ghost commented Nov 13, 2021

Uh oh!

ghost commented Nov 13, 2021

Uh oh!

bartonjs commented Nov 13, 2021

Uh oh!

safern Nov 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bartonjs Nov 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

safern left a comment

Choose a reason for hiding this comment

Uh oh!

bartonjs commented Nov 15, 2021

Uh oh!

GrabYourPitchforks commented Nov 17, 2021

Uh oh!

GrabYourPitchforks Nov 17, 2021

Choose a reason for hiding this comment

Uh oh!

bartonjs Nov 17, 2021

Choose a reason for hiding this comment

Uh oh!

bartonjs commented Nov 17, 2021

Uh oh!

Uh oh!

safern Nov 15, 2021 •

edited

Loading

bartonjs Nov 15, 2021 •

edited

Loading