- Notifications
You must be signed in to change notification settings - Fork5.2k
One shot CBC#55184
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
One shot CBC#55184
Uh oh!
There was an error while loading.Please reload this page.
Conversation
ghost commentedJul 5, 2021
Note regarding the 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 commentedJul 5, 2021
Tagging subscribers to this area:@bartonjs,@vcsjones,@krwq,@GrabYourPitchforks Issue DetailsThis implements the CBC one shots. It should be noted that the implementation could be "better" and avoid a few allocations, such as around copying the initialization vector, which will likely be for .NET 7 since it's somewhat non-trivial. This work is already done in#55090 and will be re-opened after the branch for 6.0 is taken. This also did a significant refactoring of the unit tests to avoid a cross product of duplicated code between modes and algorithms. This will make the CFB work much easier. Work remaining:
|
vcsjones commentedJul 5, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Opening as draft to get a run through CI |
vcsjones commentedJul 6, 2021
Seems like failures are either infrastructure or unrelated. Marking for review. |
...ests/System/Security/Cryptography/AlgorithmImplementations/Symmetric/SymmetricOneShotBase.cs OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
| Assert.True(result, "TryDecrypt"); | ||
| Assert.Equal(destinationBuffer.Length, bytesWritten); | ||
| AssertPlaintexts(plaintext, destinationBuffer.ToArray(), padding); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Seems like the helpers could be spanified. But doesn't really matter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Or just split the array/span versions of destination:
-Span<byte> destinationBuffer = new byte[expectedPlaintextSize];+byte[] destination = new byte[expectedPlaintextSize];+Span<byte> destinationBuffer = destination;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I kept the assert helpers as taking arrays because asserting on arrays is nicer with Xunit. Taking aReadOnlySpan<byte> would mean doing anAssert.True(blah.SequenceEqual(otherBlah)) which doesn't have quite as useful output when the assertion fails.
Since it is test code, I find the better assertion output more valuable than less allocating.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yeah. And the version for strings is nicer than the one for arrays (particularlybyte[])... at least in the past the array mismatch one was most useful if it was in the first few elements. That's why there are so many Asserts over foo.ByteArrayToHex().
One of these days I should make an assert-equality routine and clean up all of the things to do non-allocated SequenceEqual then fall back to making the string on failure. Maybe it'd shave a non-trivial amount of time off of the test execution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Oh. Actually it looks like we have one onAssertExtensions. Let me play with that a bit...
| Assert.Equal(actual, expected); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Consider a "test" to make sure the derived types all specified all the test methods:
| [Fact] | |
| publicvoidTestsDefined() | |
| { | |
| conststringDriverSuffix="Driver"; | |
| TypeimplType=GetType(); | |
| TypedefType=typeof(NonCryptoHashTestDriver); | |
| List<string>?missingMethods=null; | |
| foreach(MethodInfoinfoindefType.GetMethods(BindingFlags.Instance|BindingFlags.NonPublic)) | |
| { | |
| if(info.IsFamily&&info.Name.EndsWith(DriverSuffix,StringComparison.Ordinal)) | |
| { | |
| stringtargetMethodName=info.Name.Substring(0,info.Name.Length-DriverSuffix.Length); | |
| MethodInfoinfo2=implType.GetMethod( | |
| targetMethodName, | |
| BindingFlags.Instance|BindingFlags.Public); | |
| if(info2isnull) | |
| { | |
| missingMethods??=newList<string>(); | |
| missingMethods.Add(targetMethodName); | |
| } | |
| } | |
| } | |
| if(missingMethodsis notnull) | |
| { | |
| Assert.Empty(missingMethods); | |
| } | |
| } |
vcsjones commentedJul 6, 2021
@bartonjs some last second test improvements:
|
bartonjs commentedJul 6, 2021
Yeah. Maybe one day we'll slip in a specialization for byte spans that does hex strings once they're proven to be a mismatch. (At least, I usually find the hex easier to work with, being fixed width) |
bartonjs commentedJul 6, 2021
I think I looked at that like a half dozen times trying to decide if they were needed or not, torn between "but all the things flow..." and "why are there two bidirectional blocks with only one assert each?". Clearly I should have just left a comment 😄. |
vcsjones commentedJul 6, 2021
Yeah. I can do that outside of this PR. |
vcsjones commentedJul 7, 2021
Failures appear unrelated. |
bartonjs commentedJul 7, 2021
Yay. CBC was the important one, but now CFB should give is feature completion. |
Uh oh!
There was an error while loading.Please reload this page.
This implements the CBC one shots.
It should be noted that the implementation could be "better" and avoid a few allocations, such as around copying the initialization vector, which will likely be for .NET 7 since it's somewhat non-trivial. This work is already done in#55090 and will be re-opened after the branch for 6.0 is taken.
This also did a significant refactoring of the unit tests to avoid a product of duplicated code between modes and algorithms. This will make the CFB work much easier.
Contributes to#2406.
Work remaining: