Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

HashAlgorithmName one-shots #92430

Merged
merged 2 commits into from
Sep 24, 2023
Merged

Conversation

vcsjones
Copy link
Member

@vcsjones vcsjones commented Sep 21, 2023

This implements CryptographicOperations.{HashData,HmacData} and removes our internal helper in favor of using the public one.

Closes #91407.

@dotnet-issue-labeler
Copy link

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, 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 ghost assigned vcsjones Sep 21, 2023
@ghost
Copy link

ghost commented Sep 21, 2023

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

Issue Details

This implements CryptographicOperations.{HashData,HmacData} and removes out internal helper in favor of using the public one.

Closes #91407.

Author: vcsjones
Assignees: -
Labels:

area-System.Security

Milestone: 9.0.0

@@ -15,9 +15,11 @@ public class HmacMD5Tests : Rfc2202HmacTests<HmacMD5Tests.Traits>
public sealed class Traits : IHmacTrait
{
public static bool IsSupported => true;
public static int HashSizeInBytes => HMACSHA1.HashSizeInBytes;
public static int HashSizeInBytes => HMACMD5.HashSizeInBytes;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This turned out to be a mistake in our HMACMD5 tests but it went unnoticed because the value is only used for "create a buffer that is at least large enough". LEN(SHA1) > LEN(MD5) so it happened to work. But let's fix it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The HmacTests are in desperate need of some refactoring and love in my opinion. I will look at that hopefully sometime for .NET 9. I would like to re-model the tests similarly to how we did SHAKE.

For now though, I decided to keep the changes small so as to not make too many test changes at the same time as product changes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though the implementation is on CryptographicOperations, the tests are all part of the hashing / HMAC testing infrastructure. Otherwise we would be duplicating a lot of additional test logic, KATs, etc.

@vcsjones
Copy link
Member Author

These native AOT outerloop checks looks like they ran errantly according to #92432, so though they are failing, I marked this ready for review.

Also change the variable name for the hash length to be consistent accross all methods.
@stephentoub
Copy link
Member

Thanks.

@stephentoub stephentoub merged commit 4101144 into dotnet:main Sep 24, 2023
@vcsjones vcsjones deleted the hashalgname-oneshot branch October 7, 2023 01:11
@ghost ghost locked as resolved and limited conversation to collaborators Nov 6, 2023
@bartonjs bartonjs added the cryptographic-docs-impact Issues impacting cryptographic docs. Cleared and reused after documentation is updated each release. label Nov 28, 2023
@bartonjs bartonjs added the tracking This issue is tracking the completion of other related issues. label Aug 15, 2024
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
area-System.Security cryptographic-docs-impact Issues impacting cryptographic docs. Cleared and reused after documentation is updated each release. new-api-needs-documentation tracking This issue is tracking the completion of other related issues.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[API Proposal]: Hash and HMAC one-shots for HashAlgorithName
3 participants