Refactor symmetric one-shots #55090
Refactor symmetric one-shots #55090
Conversation
ghost
added
the
|
Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq, @GrabYourPitchforks Issue DetailsThis is a refactoring of the existing one-shot cryptography for the up-coming CBC/CFB implementations. The goal of the refactoring was to reduce allocations and do less work that the one shots don't need to do.
The CNG implementations (
|
vcsjones
changed the title
|
There is still some clean up and other ways to improve but this was getting to be a big change. In particular, the way CFB feedback size is handled is a little quirky and deserves its own PR that would be easier to review in isolation. |
| using System.Diagnostics; | ||
| using System.Security.Cryptography; | ||
|
|
||
| namespace Internal.Cryptography |
There was a problem hiding this comment.
I want to get all the stuff in Internal\Cryptography out (it should just be S.S.Cryptography). So please use System.Security.Cryptography
There was a problem hiding this comment.
Should the file move elsewhere or just fix the namespace?
|
|
||
| namespace Internal.Cryptography | ||
| { | ||
| internal interface ILiteSymmetricCipher : IDisposable |
There was a problem hiding this comment.
Lite as a prefix seems odd.
- ISpanSymmetricCipher
- ISymmetricCipherLite
- ISymmetricCipherSlim
|
|
||
| namespace Internal.Cryptography | ||
| { | ||
| internal interface ILiteSymmetricCipher : IDisposable |
There was a problem hiding this comment.
Do we need an interface here? Any reason it can't be done on BasicSymmetricCipher?
There was a problem hiding this comment.
Any reason it can't be done on BasicSymmetricCipher?
BasicSymmetricCipher has a property for the IV which is one of the things I was trying to avoid. We could just have a null IV, but I was trying to be super careful about the readability of this. One could assume "no IV means it is ECB" when really what we were intending was a CBC with no captured IV since it is not reusable.
The exception to this is the BCrypt implementation. It has no "Lite" implementation because we are responsible for passing in the IV for each call. So the BCrypt implementation just implements the interface without extracting the functionality.
| using System.Diagnostics; | ||
| using System.Security.Cryptography; | ||
|
|
||
| namespace Internal.Cryptography |
There was a problem hiding this comment.
Same general "don't add to this bad namespace, please" (unless it's just splitting a class into partials)
|
This is... pretty big. (Bigger than I have time to finish reviewing today). What's the cost if we don't do this for this release? Worst case, the CFB implementations could be using (xform = CreateTransform(CFB))
{
xform.TransformFinalBlock(...);
I assume that now we need to copy stuff?
}which gets the API out the door and then we can factor stuff out in late August to get cleaner for 7. Or if it's just "oh, it takes longer than 10 minutes" then I can give it the longer than 10 minutes next week :) |
Nothing extremely important. It just nudges us closer to an "ideal" state for the one shots in terms of allocation and not doing unnecessary work. Agree I let this one run away from me a bit. I'll close this out and keep the branch so it can re-evaluated when you're accepting things for 7.0. |
2 participants
This is a refactoring of the existing one-shot cryptography for the up-coming CBC/CFB implementations. The goal of the refactoring was to reduce allocations and do less work that the one shots don't need to do.
Reseton finalize since the one shot is not going to get used another time.ReadOnlySpan<byte>where possible.The CNG implementations (
AesCngand friends) still uses the existing one-shots on the Universal transforms. I would like to remove that in a potential follow up. Then we can remove the one-shots from the universal transform.