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

feat: Switch from AES-GCM to XChaCha20-Poly1305 #305

Merged
merged 3 commits into from
Jul 11, 2023

Conversation

matheus23
Copy link
Member

Summary

This PR

  • Switches out the aes-gcm lib for chacha20poly1305
  • Switched the Aes256Gcm struct for XChaCha20Poly1305
  • Switched the nonces from aes::Nonce to XNonce
  • Renamed AesError into CryptError (We still use AES-KW, so a more generic name made sense)
  • Removed aes.rs file with AesKey struct
  • Moved some custom key wrap/unwrap code from PrivateRef::to_serializable and PrivateRef::from_serializable to just use TemporalKey::key_wrap_{encrypt,decrypt}

The motivation for switching is simply the extended nonce. It let's us always generate a random nonce for every ciphertext and sleep well at night.
The canonical downside of not using AES-based ciphers is performance in case AES native instructions (AESNI) are available. However, this is not our main bottleneck & we'll run in lots of environments (e.g. Wasm) that don't benefit from AESNI.

@matheus23 matheus23 requested a review from a team as a code owner July 7, 2023 18:04
@matheus23 matheus23 self-assigned this Jul 7, 2023
@expede
Copy link
Member

expede commented Jul 7, 2023

Screenshot 2023-07-07 at 11 04 50

@codecov
Copy link

codecov bot commented Jul 7, 2023

Codecov Report

Merging #305 (e4ba8f9) into main (7026a37) will decrease coverage by 0.04%.
The diff coverage is 69.69%.

❗ Current head e4ba8f9 differs from pull request most recent head 6383542. Consider uploading reports for the commit 6383542 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #305      +/-   ##
==========================================
- Coverage   58.31%   58.28%   -0.04%     
==========================================
  Files          43       42       -1     
  Lines        3097     3073      -24     
  Branches      744      740       -4     
==========================================
- Hits         1806     1791      -15     
+ Misses        800      793       -7     
+ Partials      491      489       -2     
Impacted Files Coverage Δ
wnfs/src/private/encrypted.rs 66.66% <ø> (ø)
wnfs/src/private/forest/traits.rs 100.00% <ø> (ø)
wnfs/src/private/node/header.rs 76.92% <ø> (ø)
wnfs/src/private/keys/privateref.rs 56.66% <25.00%> (-0.10%) ⬇️
wnfs/src/private/node/keys.rs 82.05% <72.00%> (+7.05%) ⬆️
wnfs/src/private/file.rs 78.69% <100.00%> (ø)

Copy link
Member

@expede expede left a comment

Choose a reason for hiding this comment

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

🙌

Huzzah! LGTM!

@matheus23 matheus23 mentioned this pull request Jul 11, 2023
6 tasks
@matheus23 matheus23 merged commit c17f6bb into main Jul 11, 2023
@matheus23 matheus23 deleted the matheus23/xchacha-poly1305 branch July 11, 2023 15:50
@github-actions github-actions bot mentioned this pull request Jul 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants