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.

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

Add ChaCha20Poly1305 skeleton #52030

Merged
merged 10 commits into from
May 5, 2021

Conversation

GrabYourPitchforks
Copy link
Member

@GrabYourPitchforks GrabYourPitchforks commented Apr 29, 2021

Contributes to #45130.

This adds the skeleton ChaCha20Poly1305 API and hooks up support on Win10. The basic structure for adding non-Windows support is also present. I can't test that right now (so I didn't try), but some enterprising individual may be able to do it in a followup PR with minimal effort.

There are a few other changes here:

  • Pulled out functionality that was common to AEAD algorithms (including AES-AEAD) into their own helper classes.
  • Removed unused parameters on internal helper classes.
  • Added IsSupported APIs to AesGcm and AesCcm and hooked up unit tests for them.

The unit tests are based off the AesGcm and AesCcm tests. The expected tag and ciphertext const values were generated from a C++ program targeting the Win32 APIs directly.

Marking draft since I want to ensure the general structure is acceptable before I march forward. I leave it to the discretion of the product group at large as to whether non-Windows support should come in a followup PR or whether this should be handed off to somebody else to put the finishing touches.

This PR is currently blocked on dotnet/arcade#7316, which causes the Browser target to fail to compile.

@GrabYourPitchforks GrabYourPitchforks added area-System.Security blocked Issue/PR is blocked on something - see comments labels Apr 29, 2021
@GrabYourPitchforks GrabYourPitchforks added this to the 6.0.0 milestone Apr 29, 2021
@ghost
Copy link

ghost commented Apr 29, 2021

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

Issue Details

Contributes to #45130.

This adds the skeleton ChaCha20Poly1305 API and hooks up support on Win10. The basic structure for adding non-Windows support is also present. I can't test that right now (so I didn't try), but some enterprising individual may be able to do it in a followup PR with minimal effort.

There are a few other changes here:

  • Pulled out functionality that was common to AEAD algorithms (including AES-AEAD) into their own helper classes.
  • Removed unused parameters on internal helper classes.
  • Added IsSupported APIs to AesGcm and AesCcm and hooked up unit tests for them.

Marking draft since I want to ensure the general structure is acceptable before I march forward. I leave it to the discretion of the product group at large as to whether non-Windows support should come in a followup PR or whether this should be handed off to somebody else to put the finishing touches.

This PR is currently blocked on dotnet/arcade#7316, which causes the Browser target to fail to compile.

Author: GrabYourPitchforks
Assignees: -
Labels:

area-System.Security, blocked

Milestone: 6.0.0

@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.

@vcsjones
Copy link
Member

vcsjones commented Apr 29, 2021

I leave it to the discretion of the product group at large as to whether non-Windows support should come in a followup PR

I can commit time to implement the OpenSSL side if desired. Though, it may be best to wait until the OpenSSL 3 work is complete.

new AEADTest
{
Source = Rfc8439TestVectors,
CaseId = 1,
Copy link
Member

Choose a reason for hiding this comment

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

Since at least cases 1 and 2 aren't from the same section, it'd be nice to link to the places they came from in a comment.

I think the second is https://tools.ietf.org/html/rfc8439#appendix-A.5, and the first is https://tools.ietf.org/html/rfc8439#section-2.4.2

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure. BTW if you have any other ideas for where I can find test vectors, I'm all ears. The RFC isn't exactly flush with them. :/

For reference, symcrypt uses its own test vectors.

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.

Some naming, style, and test nits... but overall looks reasonable.

@GrabYourPitchforks
Copy link
Member Author

Note: I've temporarily updated the GenFacades package (see 498dde9) because it contains a fix required for this to build on browser. This is just to make CI happy during development. Once Maestro slurps up the fixed GenFacades package and commits it, this PR will experience a merge conflict, and I can drop our local changes to the versions.props file to resolve the conflict.

@GrabYourPitchforks
Copy link
Member Author

Next iteration will revert the GenFacades change now that maestro has updated our dependencies.

@GrabYourPitchforks GrabYourPitchforks marked this pull request as ready for review May 5, 2021 18:47
@GrabYourPitchforks GrabYourPitchforks removed the blocked Issue/PR is blocked on something - see comments label May 5, 2021
@runfoapp runfoapp bot mentioned this pull request May 5, 2021
@GrabYourPitchforks GrabYourPitchforks merged commit a50dcc8 into dotnet:main May 5, 2021
@GrabYourPitchforks GrabYourPitchforks deleted the chachapoly branch May 5, 2021 21:25
@ghost ghost locked as resolved and limited conversation to collaborators Jun 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants