Skip to content
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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

Hmac one-shot #53487

Merged
merged 8 commits into from
Jun 4, 2021
Merged

Hmac one-shot #53487

merged 8 commits into from
Jun 4, 2021

Conversation

vcsjones
Copy link
Member

Implements HMAC one-shots for Linux, macOS, Windows, and Android.

Closes #40012

@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, 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
Copy link

ghost commented May 30, 2021

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

Issue Details

Implements HMAC one-shots for Linux, macOS, Windows, and Android.

Closes #40012

Author: vcsjones
Assignees: -
Labels:

area-System.Security, new-api-needs-documentation
Milestone: -

@vcsjones
Copy link
Member Author

Note: updates to use the one-shot where appropriate throughout libraries will be done in a follow-up PR.

@vcsjones vcsjones marked this pull request as ready for review May 31, 2021 01:31
filipnavara
filipnavara reviewed May 31, 2021
View reviewed changes
src/libraries/System.Security.Cryptography.Algorithms/tests/HmacMD5Tests.cs

[Fact]
public void HmacMD5_Rfc2202_1()
{
VerifyHmac(1, "9294727a3638bb1c13f48ef8158bfc9d");
VerifyHmac(1, s_testMacs2202[1]);
Copy link
Member

Choose a reason for hiding this comment

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

IMHO this would have been more readable as single parametrized test. Then again, it is separate cleanup anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the tests could do with some cleanup. But I was trying my best to avoid re-writing the tests from scratch. Maybe I can do this in a follow up PR, unless someone feels strongly that "now is the time".

Copy link
Member

Choose a reason for hiding this comment

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

I think I'm happy with what I came up with for non-crypto hashing tests (#53623). This could converge to something like that in a subsequent change.

I wouldn't want to hold up the API part on the test restructuring, though.

bartonjs
bartonjs reviewed Jun 3, 2021
View reviewed changes
Copy link
Member

@bartonjs bartonjs left a comment

Choose a reason for hiding this comment

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

Overall, looks good. Just a couple small style (code and design) things.

@bartonjs bartonjs merged commit 3188dcd into dotnet:main Jun 4, 2021
@vcsjones vcsjones deleted the hmac-oneshot branch June 4, 2021 20:17
@ghost ghost locked as resolved and limited conversation to collaborators Jul 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@filipnavara filipnavara filipnavara left review comments

@bartonjs bartonjs bartonjs approved these changes

Assignees
No one assigned
Labels
area-System.Security new-api-needs-documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

HMAC one-shot methods
3 participants
@vcsjones @filipnavara @bartonjs