Merge all of the inbox S.S.C.* libraries into one #55690
Comments
dotnet-issue-labeler
bot
added
area-System.Security
untriaged
|
Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq, @GrabYourPitchforks Issue DetailsCurrently, the This has already impacted our ability to do something we wanted to once already at #34086 (comment). With #51630, we’re going to run in to this again, with a stronger argument that the PKCS8 exports should be done on
|
I think that it is, it's just more complicated 😄. private delegate int TryWriteDelegate(ReadOnlySpan<char> label, ReadOnlySpan<byte> data, Span<char> destination, out int charsWritten);
...
Type t = ...;
MethodInfo method = t.GetMethod("TryWrite", BindingFlags.Static | BindingFlags.Public);
TryWriteDelegate twd = (TryWriteDelegate)method.CreateDelegate(typeof(TryWriteDelegate));
// squirrel that off into a field somewhere.There've been a few things that make me feel like we might want to just merge everything except Pkcs and Xml into System.Private.Security.Cryptography.dll, but maybe it doesn't need to be quite that drastic, and this one type is all we should move. |
If that's on the table I'd very much prefer that. I can't recall the specifics but on multiple occasions I wished it were a big assembly. I'm sure if I went digging through some of my abandoned PRs I could jog my memory. 😄 |
On Symmetric? PKCS#8 doesn't make sense there. On Asymmetric? We define the methods there, but since each algorithm has unique ways for doing the algorithm identifier (IIRC that's where DSA shoves the domain parameters and EC shoves the curve declaration) and the payload, there's not a lot that the base class can really do without there being two protected virtuals... so we let the algorithms do it with the one. |
Yeah typed the wrong thing there. It seems some part of my brain is still working on one shots.
For export, the only thing on |
Ooooh, not the real PKCS8, but the PEM-PKCS8. Yeah, since that's just "call the existing PKCS8 virtual, PEM-ify the result". |
Yeah, I could have been more clear what I was talking about 😄 Maybe a place to start then is to make a new System.Security.Cryptography project and begin with moving .Encoding and .Primitives in there. We can move the remaining later, if it makes sense to move them. .Xml and .Pkcs should probably stay out, and probably .ProtectedData? |
|
@ericstj Anything special that should be kept in mind while considering collapsing the non-NuGet crypto libraries? I feel like there are probably two approaches:
I don't think, off the top of my head, we need the layering for any co-dependency reasons. It was mostly a Core 1.0 artifact. In extremis:
I believe that all 7 are included in the shared framework on all OSes. |
|
There are some "reflection-y" things that happen with |
|
Yep, it works. |
Ah, thanks.
Are there any resource on the benefits of the first approach? I know that's done in some places like CoreLib, Uri, and Xml, but I don't quite see why. |
|
Another issue we may be able to close if we merge these assemblies is #29555. |
|
Keep in mind TypeForwardedFrom for any serializable types (hopefully not an issue). You will of course need to keep the old assemblies as facades for compatibility but those can be a one-time cost and effectively ignored after that. You still may want to enforce some layering in your own codebase but that can be done through convention and code review. You may end up with PNSEs for surface area that’s smaller than the assembly. Today you benefit from the PNSE generator but you’ll need to either check that in or we may need a better partial PNSE solution. |
I chatted with Eric via a different medium, and concluded that System.Private.* isn't the route we want to take. The System.Private.* libraries started when we had the small contract assemblies in .NET Core 1.0. Whenever we felt like it was important (or maybe just expedient) to have multiple contracts backed by one implementation assembly a System.Private was born. The feeling back then, IIRC, was "if you can avoid it, please avoid it". So the System.Private.* assemblies seem to, largely, represent "we wish we could sensibly split these apart, and want to make less work on ourselves when we do so". Since we're instead saying "we don't think it makes sense to keep these split apart", we don't need the "Private" artifact. (And if I misunderstood, hopefully someone corrects me) |
bartonjs
changed the title
|
With the stage 1 PR I got to avoid platform-differentiated code. Stage 2 is being less nice. We've evolved ourselves into having a couple of different "standards" in file pathing for how we have platform splits. For example:
Note that sometimes Apple is split into macOS and iOS (and in one case tvOS), but not in these examples. In addition to getting rid of "Internal\Cryptography" as part of this move, it feels like we should try to come up with a better path/name scheme. I think my strawman is going to be
iOS/macOS could also easily be subdirectories under Apple, or extension variants (e.g. System\Security\Cryptography\Pal\Apple\RSASecurityTransforms.macOS.cs) The idea behind this strawman being that we name the directories less about where they run "Unix" and more about what crypto provider they're working with "Windows", "OpenSSL", "Apple" (Security.framework), "Android" (Android JDK/JNI), and "Managed" (not PAL'd). So for the earlier example files:
Not sure I'm happy with that, though, so definitely open to feedback. @stephentoub I'd be happy to get your insight/opinion here. |
|
"open mic" style feedback...
Having a managed "PAL" seems odd.
I really struggle with files with the same name in different directories. It makes code navigation harder (Cmd+P) and it when looking at a tab row of many open files, they all have the same name so I just end up lost. I greatly prefer file-based suffixes instead of directories that contain a handful of files with lots of overlapping names. If that is not "the way", so be it, but it's one of the things that has bugged me quite a bit. |
|
That’s a good point. It has also frustrated me, but somehow that didn’t occur to me when I was strawmanning. |
|
Once the files have unique names, I guess I should question whether there's value in PAL directories at all. The problem I have with them a bit right now is that "Android" can be in both "Android" and "Unix", but Linux is only in "Unix", though not necessarily all of Unix (since that could also be something shared by Apple and Android); so if they're present it feels like they should be about the provider, but I'm open to no-directory, too. No extra directories would then end up with something like
|
|
@vcsjones: Since Encodings just got folded in to the new library, AsymmetricAlgorithm and PemEncoding are in the same library now. Since that was the catalyst for the squishification, thought I'd point that out explicitly. |
+1. This is a key reason we went with *.Suffix.cs. Can we get rid of the "\Pal" directory as well? Doesn't seem to be adding much benefit. Ideally we end up with all the partials right next to each other, e.g. |
Oh, good, that's the approach I'm currently pursuing with squishing Algorithms down into the unified library 😄. |
|
Closed via #64307. |
ghost
locked as resolved and limited conversation to collaborators
Currently, the
PemEncodingandPemFieldstypes live in System.Security.Cryptography.Encoding. This made sense at the time. However, it makes it difficult to use the PEM functionality from things inSystem.Security.Cryptography.Primitives, specificallyAsymmetricAlgorithm, because that would be a circular dependency.This has already impacted our ability to do something we wanted to at #34086 (comment).
With #51630, we’re going to run in to this again, with a stronger argument that the PEM-PKCS8 exports should be done on
AsymmetricAlgorithmdirectly. This leaves us with two options:Primitivesassembly.This is doable, but we will need to call the allocating APIs since working withSpan<T>with reflection is not doable today.The text was updated successfully, but these errors were encountered: