Fold inbox crypto into one assembly #64307

bartonjs · 2022-01-26T01:25:26Z

This change does not move any of the tests associated with the code moves.

For System.Security.Cryptography.Csp.Tests.dll,
System.Security.Cryptography.Cng.Tests.dll, and
System.Security.Cryptography.OpenSsl.Tests.dll, the crypto test infrastructure needs
to be updated to allow for multiple algorithm providers to be tested at once.
Currently, those test libraries have static provider/factory types that say that (e.g.)
the RSA to test is RSA.Create() / RSACryptoServiceProvider / RSACng / RSAOpenSsl.

For System.Security.Cryptography.X509Certificates.Tests.dll, the tests could be moved,
but require a different level of thinking for path and namespace unification than was
required for previous phases. So, for consistency within this PR, the X509 tests also
aren't moving yet.

This unification revealed several places were our OS support attributes weren't quite right,
due to implementations being automatically generated as PNSE.

Constructors of CNG types all gained [SupportedOS("windows")]
Constructors of OpenSsl types all gained [UnsupportedOS("android")] / browser / ios / tvos / windows (works on macOS and linux)
Constructors and static methods on X509Certificate(2) and the X509Chain.Build method gained [UnsupportedOS("browser")]
The CertificateRequest class gained [UnsupportedOS("browser")]. It's unlikely to be used as an exchange type, so the type got painted instead of constructors.

dotnet-issue-labeler · 2022-01-26T01:25:37Z

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 · 2022-01-26T01:25:42Z

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

Issue Details

This change does not move any of the tests associated with the code moves.

For System.Security.Cryptography.Csp.Tests.dll,
System.Security.Cryptography.Cng.Tests.dll, and
System.Security.Cryptography.OpenSsl.Tests.dll, the crypto test infrastructure needs
to be updated to allow for multiple algorithm providers to be tested at once.
Currently, those test libraries have static provider/factory types that say that (e.g.)
the RSA to test is RSA.Create() / RSACryptoServiceProvider / RSACng / RSAOpenSsl.

For System.Security.Cryptography.X509Certificates.Tests.dll, the tests could be moved,
but require a different level of thinking for path and namespace unification than was
required for previous phases. So, for consistency within this PR, the X509 tests also
aren't moving yet.

This unification revealed several places were our OS support attributes weren't quite right,
due to implementations being automatically generated as PNSE.

Constructors of CNG types all gained [SupportedOS("windows")]
Constructors of OpenSsl types all gained [UnsupportedOS("browser")] / ios / tvos / windows (works on macOS and linux)
Constructors and static methods on X509Certificate(2) and the X509Chain.Build method gained [UnsupportedOS("browser")]
The CertificateRequest class gained [UnsupportedOS("browser")]. It's unlikely to be used as an exchange type, so the type got painted instead of constructors.

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

bartonjs · 2022-01-26T01:28:52Z

src/libraries/Common/src/System/Security/Cryptography/DSACng.ImportExport.cs

@@ -11,432 +11,425 @@

 namespace System.Security.Cryptography
 {
-#if INTERNAL_ASYMMETRIC_IMPLEMENTATIONS
-    internal static partial class DSAImplementation
+    public sealed partial class DSACng : DSA


All of the implementation files that are in libraries/Common/src/... could be moved to under System.Security.Cryptography at this point; but I chose not to move them at this time to keep the PR a bit more... sane.

bartonjs · 2022-01-26T01:34:12Z

src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/CngKey.EC.cs

@@ -115,14 +112,17 @@ internal static CngAlgorithm EcdhCurveNameToAlgorithm(string name)
            {
                case "nistP256":
                case "ECDH_P256":
+                case "ECDSA_P256":


There were two copies of these, one had this line, the other didn't. Apparently one got hit by ECDiffieHellman.Create(), the other git hit by ECDiffieHellmanCng.

bartonjs · 2022-01-28T01:50:34Z

@vcsjones / @filipnavara The failing tests all seem to look like

using (ECDsa key = ECDsa.Create(...))
{
    Make/get a cert from somewhere
    cert.CopyWithPrivateKey(key);  // <-- fails
}

This is because previously the as check in CopyWithPrivateKey failed for ECDsa.Create() (it was an ECSecurityTransforms, but from a different assembly, so the as said no/null).

Presumably the failure is that either
a) we never tested cert.CopyWithPrivateKey using an ECC key from a keychain (aka we never tested the as being true)
b) ECDsa.Create() and cert.GetECDsaPrivateKey() are doing something different (some other ephemeral key vs keychain keys?)

Having gotten that far in the investigation my brain is sort of fried, so looking for easy pointers on how to update the check (

runtime/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.OSX/AppleCertificatePal.cs

Lines 490 to 496 in afa4b2f

    
           public ICertificatePal CopyWithPrivateKey(ECDsa privateKey) 
        
           { 
        
               var typedKey = privateKey as ECDsaImplementation.ECDsaSecurityTransforms; 
        
               if (typedKey != null) 
        
               { 
        
                   return CopyWithPrivateKey(typedKey.GetKeys());

) easily. I mean... I could easily make ECDsa.Create() and ECDiffieHellman.Create() both use the wrapper type that CNG/OpenSsl use... that'd probably hide the problem in the short term... but if there's an easier check, I'd love to know what it is.

Interestingly, RSA doesn't have that problem, and either DSA doesn't or we don't test DSA.

vcsjones · 2022-01-28T02:03:02Z

@bartonjs I can take a look Friday / Saturday. This rings a bell when I spiked out the key services changes.

vcsjones · 2022-01-28T02:11:21Z

Sounds like we're running in to the situation I ran in to in the 3rd paragraph of my spike: #38101

filipnavara · 2022-01-28T06:07:26Z

It definitely rings a bell. I think I intentionally ignored the scenario because it couldn't happen when the implementations were in different assemblies. Mixing the two types of keys (keychain/CSSM vs. the new iOS-style one) just doesn't work in many cases so recreating the ephemeral keypair by export+import is probably the best that can be done. It should also result in the fast native implementation being used. The CopyWithPrivateKey(SecKeyPair) code path would work only if both keys come from keychain.

Essentially it boils down to couple of options:

Removing the CopyWithPrivateKey(SecKeyPair) code path altogether for ECC. Presumably it doesn't make much sense to use two implementations from keychain here. Even if you would then the export+import code path would result in the faster ECC implementation being used.
Tracking in ECDsaSecurityTransforms which type of key it uses and keeping the CopyWithPrivateKey(SecKeyPair) code path only when both keys originated from the keychain.
Detecting the failure in CopyWithPrivateKey(SecKeyPair) and doing a fallback to export+import.
Updating CopyWithPrivateKey(SecKeyPair) to do the export+import fallback in the native code.

Interestingly, RSA doesn't have that problem...

The Apple's code is much more forgiving when mixing the legacy CSSM keys and the iOS-style keys with RSA. However, it could be a hidden performance trap. If the CopyWithPrivateKey(SecKeyPair) code path succeeds now it would likely return an implementation backed by the slow CSSM keys. It's likely not going to be a huge issue but definitely something to be aware of. Nevermind, the performance trap is non-existant since it ends up in ImportEphemeralKey anyway and thus always creates CSSM keys.

...and either DSA doesn't or we don't test DSA.

DSA has only single legacy implementation (CSSM) in the Apple's native code. There's no DSA in iOS and also no iOS-style DSA keys on macOS. Thus SecKeyRef always uses the same backing implementation and doesn't run into problems.

filipnavara · 2022-01-28T08:11:12Z

Here's a proof of concept fix: ed16db6

bartonjs · 2022-01-28T16:44:22Z

Removing the CopyWithPrivateKey(SecKeyPair) code path altogether for ECC. Presumably it doesn't make much sense to use two implementations from keychain here.

Well... that path would be the only one that works if you have a non-exportable key stored in a keychain. Since we don't expose the Apple interop types as public API the only way to get one of those is from GetECDsaPrivateKey(). (Which, basically limits the scenario to "I want to renew/regen a cert with the same key", I suppose)

Thanks for the workaround code. I don't like duplicating the logic of the "internal representation" of Apple EC keys, but I think I see a path forward from that.

filipnavara · 2022-01-28T16:59:06Z

Well... that path would be the only one that works if you have a non-exportable key stored in a keychain. Since we don't expose the Apple interop types as public API the only way to get one of those is from GetECDsaPrivateKey(). (Which, basically limits the scenario to "I want to renew/regen a cert with the same key", I suppose)

I had the same line of thinking and went back and forth with striking the option from the list :-) Eventually I left it there to paint the full picture so someone without sleep deprivation can re-read it...

Thanks for the workaround code. I don't like duplicating the logic of the "internal representation" of Apple EC keys, but I think I see a path forward from that.

Not a big fan of it myself but I wanted to unblock the PR.

I've tried to implement other options from the list but they were quite a big mess. The iOS-style keys only support X.963 format export, the legacy SecItemExport API doesn't work for them at all. That basically ruled out handling the situation in native code because it would get too complex. So next best thing is handling it in the managed part of the code and taking different path for iOS-style keys and the CSSM ones. The key type can be inferred by calling SecKeyCopyExternalRepresentation, fails for CSSM keys and succeeds for iOS-style ones. It's a relatively cheap call that just copies an array but it felt wrong to just throw away the array.

bartonjs · 2022-01-28T18:14:43Z

Instead of duplicating the data format handler, I factored it out in 1243105. More total lines, but gives me a bit of a warm fuzzy feeling.

filipnavara · 2022-01-28T20:17:00Z

Tests look good

This change leaves the tests in System.Security.Cryptography.Csp.Tests.dll, because the crypto test infrastructure has not yet been upgraded to allow for two algorithm provider tests in the same assembly (e.g. not both Aes.Create() and AesCryptoServiceProvider can be tested)

bartonjs · 2022-01-28T20:46:47Z

Rebased for merge conflicts... again... hopefully for the last time.

src/libraries/System.Security.Cryptography/ref/System.Security.Cryptography.cs

vcsjones · 2022-01-29T00:25:51Z

src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/CapiHelper.Unix.cs

@@ -94,7 +91,7 @@ public static HashAlgorithmName NameOrOidToHashAlgorithmName(string nameOrOid)
            return null;
        }

-        public static HashAlgorithmName? OidToHashAlgorithmName(string oid)
+        internal static HashAlgorithmName? OidToHashAlgorithmName(string oid)


I think this can be replaced entirely by HashAlgorithmName.TryFromOid now. Don't have to do it here, just leaving a note as something one of us can do afterward.

vcsjones · 2022-01-29T00:50:55Z

...ries/System.Security.Cryptography/src/System/Security/Cryptography/ECDiffieHellmanWrapper.cs

+        // Do not wrap ExportPkcs8PrivateKey, let it fall back to reconstructing it from parameters
+        // so that the ECDiffieHellman.Create()-returned object uses the same set of attributes on all platforms.
+        // (CNG adds the key usage attribute to distinguish ECDSA from ECDH)
+        //public override byte[] ExportPkcs8PrivateKey() => _wrapped.ExportPkcs8PrivateKey();


Interesting. It this just so we get bit-for-bit (or thereabouts) the same exports on platforms? Seems reasonable, but is there now a discrepancy between this attribute's presence between encrypted and non-encrypted PKCS8 exports?

Hm, you're right, I might need to block the encrypted ones as well.

We had a test for the unencrypted ones. Encrypted export from CNG is special, since they have complicated export permissions... but that shouldn't apply for ECDsa.Create() keys.

vcsjones · 2022-01-29T00:52:52Z

...em.Security.Cryptography/src/System/Security/Cryptography/ECDsa.Create.SecurityTransforms.cs

+        public static partial ECDsa Create(ECParameters parameters)
+        {
+            ECDsa ecdsa = Create();
+            ecdsa.ImportParameters(parameters);


Do we want to catch, dispose, re-throw here?

vcsjones · 2022-01-29T00:53:15Z

...raries/System.Security.Cryptography/src/System/Security/Cryptography/ECDsa.Create.Windows.cs

+        public static partial ECDsa Create(ECParameters parameters)
+        {
+            ECDsa ec = new ECDsaCng();
+            ec.ImportParameters(parameters);


Same catch, dispose, re-throw question.

vcsjones · 2022-01-29T00:55:38Z

src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/ECDsaCng.cs

-            ECDsa ec = new ECDsaImplementation.ECDsaCng();
-            ec.ImportParameters(parameters);
-            return ec;
+            _core.Dispose();


Should this be inside if (disposing)? Is finalizer somewhere going to mean _core.Dispose() going to get disposed twice?

For sanity/consistency, probably. But since the Cng classes are sealed, there's not a sneaky place a finalizer can come in. While it looks like I massively rewrote this file, it's really "move the one from the other project over top this", where the Dispose already looked like that.

vcsjones · 2022-01-29T01:00:38Z

src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/RSACng.cs


+        protected override void Dispose(bool disposing)
+        {
+            _core.Dispose();


Another if (disposing) question.

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

vcsjones · 2022-01-29T01:08:34Z

Aside from the ref SHA512CryptoServiceProvider, everything else is just some questions or nits.

vcsjones

I wouldn't consider any of my remaining feedback blocking. It can be done in follow up PRs, if chosen to be done at all.

danmoseley · 2022-01-30T02:04:11Z

src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/Cng.NotSupported.cs

+
+        protected abstract bool ReleaseNativeHandle();
+    }
+    public sealed partial class SafeNCryptKeyHandle : SafeNCryptHandle


nit missing newline before type

and below..

danmoseley · 2022-01-30T02:09:19Z

src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/ECDsaCng.cs

        {
-            return new ECDsaImplementation.ECDsaCng();
+            get


someone can change these to expression bodied form someday.

danmoseley

Is there a note somewhere explaining the resaoning behind these moves, as I'm out of the loop?

vcsjones · 2022-01-30T02:26:48Z

@danmoseley the issue for this is at #55690.

ericstj · 2022-01-31T21:01:03Z

@ViktorHofer -- seems like this should reduce the number of P2Ps we require in the repo if I'm not mistaken. Could be interesting for the build perf work. @bartonjs -- did you consider removing P2Ps to assemblies which are now full facades from other projects in the repo?

bartonjs · 2022-01-31T22:32:21Z

@ericstj I feel like I tried it in the earlier phases and almost everything had to be put back (might be observation bias error), so I didn't look into it for this phase. But now that it's complete I can believe there's probably some reduction to be had.

EgorBo · 2022-02-07T23:50:27Z

@bartonjs I'm seeing noticeable improvements for "publish size", this PR is the only suspect 👍

https://aka.ms/aspnet/benchmarks
10th page,

ghost assigned bartonjs Jan 26, 2022

dotnet-issue-labeler bot added area-System.Security new-api-needs-documentation labels Jan 26, 2022

bartonjs removed the new-api-needs-documentation label Jan 26, 2022

bartonjs commented Jan 26, 2022

View reviewed changes

bartonjs force-pushed the fold_inbox_crypto branch from 87be37c to 1243105 Compare January 28, 2022 18:10

bartonjs added 12 commits January 28, 2022 12:36

Change CryptoConfig string lookups to typeof for CSP types

e6f9000

Fold S.S.C.Cng into S.S.Cryptography

3cec1f0

Fold S.S.C.OpenSsl into S.S.Cryptography

e4159db

Fold S.S.C.X509Certificates into S.S.Cryptography

60d1223

Fix dependent projects

c843282

Fix ApiCompat errors

6385dd2

Self-feedback

ee7c362

Use typeof where now possible in CryptoConfig

a4c3f5b

Don't use CNG's PKCS8 export for default EC objects

e99574d

RSA should probably function on Unix

8465a78

Fix nullability state of ChainPal.BuildChain across OSes

e160422

bartonjs added 3 commits January 28, 2022 12:41

Remove bad assert

ae0c119

Special case macOS ECC data keys in CopyWithPrivateKey

bb36d3a

Undo some of the string resurrections

ad5142e

bartonjs force-pushed the fold_inbox_crypto branch from 1243105 to ad5142e Compare January 28, 2022 20:46

vcsjones reviewed Jan 28, 2022

View reviewed changes

src/libraries/System.Security.Cryptography/ref/System.Security.Cryptography.cs Show resolved Hide resolved

vcsjones reviewed Jan 29, 2022

View reviewed changes

src/libraries/System.Security.Cryptography/src/System.Security.Cryptography.csproj Show resolved Hide resolved

Add missing Obsolete attribute

449598f

vcsjones approved these changes Jan 29, 2022

View reviewed changes

danmoseley reviewed Jan 30, 2022

View reviewed changes

danmoseley approved these changes Jan 30, 2022

View reviewed changes

danmoseley merged commit c12bea8 into dotnet:main Jan 30, 2022

bartonjs deleted the fold_inbox_crypto branch January 31, 2022 00:37

bartonjs mentioned this pull request Jan 31, 2022

Merge all of the inbox S.S.C.* libraries into one #55690

Closed

TanayParikh mentioned this pull request Feb 1, 2022

[main] Update dependencies from dotnet/runtime dotnet/efcore dotnet/aspnetcore#39853

Merged

ghost locked as resolved and limited conversation to collaborators Mar 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fold inbox crypto into one assembly #64307

Fold inbox crypto into one assembly #64307

bartonjs commented Jan 26, 2022 •

edited

Loading

dotnet-issue-labeler bot commented Jan 26, 2022

ghost commented Jan 26, 2022

bartonjs Jan 26, 2022

bartonjs Jan 26, 2022

bartonjs commented Jan 28, 2022

vcsjones commented Jan 28, 2022

vcsjones commented Jan 28, 2022

filipnavara commented Jan 28, 2022 •

edited

Loading

filipnavara commented Jan 28, 2022

bartonjs commented Jan 28, 2022

filipnavara commented Jan 28, 2022

bartonjs commented Jan 28, 2022

filipnavara commented Jan 28, 2022

bartonjs commented Jan 28, 2022

vcsjones Jan 29, 2022 •

edited

Loading

vcsjones Jan 29, 2022

bartonjs Jan 29, 2022

vcsjones Jan 29, 2022

vcsjones Jan 29, 2022

vcsjones Jan 29, 2022

bartonjs Jan 29, 2022

vcsjones Jan 29, 2022

vcsjones commented Jan 29, 2022

vcsjones left a comment •

edited

Loading

danmoseley Jan 30, 2022

danmoseley Jan 30, 2022

danmoseley Jan 30, 2022

danmoseley left a comment

vcsjones commented Jan 30, 2022

ericstj commented Jan 31, 2022

bartonjs commented Jan 31, 2022

EgorBo commented Feb 7, 2022 •

edited

Loading

Fold inbox crypto into one assembly #64307

Fold inbox crypto into one assembly #64307

Conversation

bartonjs commented Jan 26, 2022 • edited Loading

dotnet-issue-labeler bot commented Jan 26, 2022

ghost commented Jan 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartonjs commented Jan 28, 2022

vcsjones commented Jan 28, 2022

vcsjones commented Jan 28, 2022

filipnavara commented Jan 28, 2022 • edited Loading

filipnavara commented Jan 28, 2022

bartonjs commented Jan 28, 2022

filipnavara commented Jan 28, 2022

bartonjs commented Jan 28, 2022

filipnavara commented Jan 28, 2022

bartonjs commented Jan 28, 2022

vcsjones Jan 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vcsjones commented Jan 29, 2022

vcsjones left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danmoseley left a comment

Choose a reason for hiding this comment

vcsjones commented Jan 30, 2022

ericstj commented Jan 31, 2022

bartonjs commented Jan 31, 2022

EgorBo commented Feb 7, 2022 • edited Loading

bartonjs commented Jan 26, 2022 •

edited

Loading

filipnavara commented Jan 28, 2022 •

edited

Loading

vcsjones Jan 29, 2022 •

edited

Loading

vcsjones left a comment •

edited

Loading

EgorBo commented Feb 7, 2022 •

edited

Loading