One shot CBC #55184

vcsjones · 2021-07-05T18:32:20Z

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:

XML docs

ghost · 2021-07-05T18:32:27Z

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-07-05T18:32:29Z

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

Issue Details

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 cross product of duplicated code between modes and algorithms. This will make the CFB work much easier.

Work remaining:

XML docs

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

vcsjones · 2021-07-05T18:34:22Z

Opening as draft to get a run through CI ~~and because the XML documentation is not done~~, but the code should be in a reviewable state.

vcsjones · 2021-07-06T04:15:49Z

Seems like failures are either infrastructure or unrelated. Marking for review.

...ests/System/Security/Cryptography/AlgorithmImplementations/Symmetric/SymmetricOneShotBase.cs

bartonjs · 2021-07-06T21:35:48Z

...ests/System/Security/Cryptography/AlgorithmImplementations/Symmetric/SymmetricOneShotBase.cs

+                Assert.True(result, "TryDecrypt");
+                Assert.Equal(destinationBuffer.Length, bytesWritten);
+
+                AssertPlaintexts(plaintext, destinationBuffer.ToArray(), padding);


Seems like the helpers could be spanified. But doesn't really matter.

Or just split the array/span versions of destination:

-Span<byte> destinationBuffer = new byte[expectedPlaintextSize]; +byte[] destination = new byte[expectedPlaintextSize]; +Span<byte> destinationBuffer = destination;

I kept the assert helpers as taking arrays because asserting on arrays is nicer with Xunit. Taking a ReadOnlySpan<byte> would mean doing an Assert.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.

Yeah. And the version for strings is nicer than the one for arrays (particularly byte[])... 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.

Oh. Actually it looks like we have one on AssertExtensions. Let me play with that a bit...

bartonjs · 2021-07-06T22:09:38Z

...ests/System/Security/Cryptography/AlgorithmImplementations/Symmetric/SymmetricOneShotBase.cs

+                Assert.Equal(actual, expected);
+            }
+        }
+    }


Consider a "test" to make sure the derived types all specified all the test methods:

runtime/src/libraries/System.IO.Hashing/tests/NonCryptoHashTestDriver.cs

Lines 33 to 63 in 57bfe47

[Fact]

public void TestsDefined()

{

const string DriverSuffix = "Driver";

Type implType = GetType();

Type defType = typeof(NonCryptoHashTestDriver);

List<string>? missingMethods = null;

foreach (MethodInfo info in defType.GetMethods(BindingFlags.Instance | BindingFlags.NonPublic))

{

if (info.IsFamily && info.Name.EndsWith(DriverSuffix, StringComparison.Ordinal))

{

string targetMethodName = info.Name.Substring(0, info.Name.Length - DriverSuffix.Length);

MethodInfo info2 = implType.GetMethod(

targetMethodName,

BindingFlags.Instance | BindingFlags.Public);

if (info2 is null)

{

missingMethods ??= new List<string>();

missingMethods.Add(targetMethodName);

}

}

}

if (missingMethods is not null)

{

Assert.Empty(missingMethods);

}

}

vcsjones · 2021-07-06T22:40:37Z

@bartonjs some last second test improvements:

Use AssertExtensions.SequenceEqual to assert spans. Its output is... acceptable.

  Error Message:
   Showing first 10 differences
 Position 23: Expected: 181, Actual: 74
 Total number of differences: 1 out of 24

While switching to AssertExtensions I noticed two asserts were missing in the roundtrip test.
Incorporated a reflection test as suggested.

bartonjs · 2021-07-06T23:26:17Z

Its output is... acceptable.

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 · 2021-07-06T23:28:05Z

While switching to AssertExtensions I noticed two asserts were missing in the roundtrip test.

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 · 2021-07-06T23:29:25Z

Maybe one day we'll slip in a specialization for byte spans

Yeah. I can do that outside of this PR.

vcsjones · 2021-07-07T02:46:26Z

Failures appear unrelated.

bartonjs · 2021-07-07T18:12:53Z

Yay. CBC was the important one, but now CFB should give is feature completion.

vcsjones added 15 commits June 30, 2021 18:30

Implement CBC one shots on SymmetricAlgorithm.

6b49e32

Naive CBC core implementation

4f03bc9

refactor AES one shot tests

78b5cbe

CBC one shot tests for AES

7e8fe07

Implement AES CBC one shot decrypt

cd3258c

CBC one shot tests

5fefa1c

Test in existing cipher tests

c819802

Refactor duplicate code in test cases

e994a84

Fixing more of my past shortcuts

76aeeaa

DES one shot and tests

42faa2c

RC2 one shot and tests

cdd0951

3DES one shot and tests

4010f46

Symmetric algorithm tests

ea15909

Add 3DES cipher tests for one shots

7183d4f

Implement CNG CBC one shots

1786f98

ghost added area-System.Security new-api-needs-documentation labels Jul 5, 2021

vcsjones marked this pull request as draft July 5, 2021 18:32

vcsjones added 5 commits July 5, 2021 19:28

XML docs

bc25a75

Implement IV validation

27b5689

Add docs for IV validation

1e1e642

Fix CSP tests

1abe236

Skip one shot tests on Browser

29d6577

vcsjones marked this pull request as ready for review July 6, 2021 04:15

bartonjs reviewed Jul 6, 2021

View reviewed changes

...ests/System/Security/Cryptography/AlgorithmImplementations/Symmetric/SymmetricOneShotBase.cs Outdated Show resolved Hide resolved

All your case are belong to us

48454b7

bartonjs reviewed Jul 6, 2021

View reviewed changes

Meta test to ensure derived types follow the testing pattern

52fdaf6

bartonjs approved these changes Jul 6, 2021

View reviewed changes

Spanify asserts

d3b85a5

bartonjs approved these changes Jul 6, 2021

View reviewed changes

bartonjs merged commit 847cc56 into dotnet:main Jul 7, 2021

vcsjones deleted the 2406-one-shot-cbc branch July 8, 2021 06:33

ghost locked as resolved and limited conversation to collaborators Aug 7, 2021

jeffhandley added area-System.IO.Hashing and removed area-System.Security labels Nov 22, 2022

	[Fact]
	public void TestsDefined()
	{
	const string DriverSuffix = "Driver";
	Type implType = GetType();
	Type defType = typeof(NonCryptoHashTestDriver);
	List<string>? missingMethods = null;

	foreach (MethodInfo info in defType.GetMethods(BindingFlags.Instance \| BindingFlags.NonPublic))
	{
	if (info.IsFamily && info.Name.EndsWith(DriverSuffix, StringComparison.Ordinal))
	{
	string targetMethodName = info.Name.Substring(0, info.Name.Length - DriverSuffix.Length);

	MethodInfo info2 = implType.GetMethod(
	targetMethodName,
	BindingFlags.Instance \| BindingFlags.Public);

	if (info2 is null)
	{
	missingMethods ??= new List<string>();
	missingMethods.Add(targetMethodName);
	}
	}
	}

	if (missingMethods is not null)
	{
	Assert.Empty(missingMethods);
	}
	}

One shot CBC #55184

One shot CBC #55184

Uh oh!

Conversation

vcsjones commented Jul 5, 2021 • edited by bartonjs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jul 5, 2021

Uh oh!

ghost commented Jul 5, 2021

Uh oh!

vcsjones commented Jul 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vcsjones commented Jul 6, 2021

Uh oh!

Uh oh!

bartonjs Jul 6, 2021

Choose a reason for hiding this comment

Uh oh!

bartonjs Jul 6, 2021

Choose a reason for hiding this comment

Uh oh!

vcsjones Jul 6, 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 Jul 6, 2021

Choose a reason for hiding this comment

Uh oh!

vcsjones Jul 6, 2021

Choose a reason for hiding this comment

Uh oh!

bartonjs Jul 6, 2021

Choose a reason for hiding this comment

Uh oh!

vcsjones commented Jul 6, 2021

Uh oh!

bartonjs commented Jul 6, 2021

Uh oh!

bartonjs commented Jul 6, 2021

Uh oh!

vcsjones commented Jul 6, 2021

Uh oh!

vcsjones commented Jul 7, 2021

Uh oh!

bartonjs commented Jul 7, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vcsjones commented Jul 5, 2021 •

edited by bartonjs

Loading

vcsjones commented Jul 5, 2021 •

edited

Loading

vcsjones Jul 6, 2021 •

edited

Loading