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

Split EncString for asymmetric support #448

Merged
merged 2 commits into from
Dec 19, 2023
Merged

Split EncString for asymmetric support #448

merged 2 commits into from
Dec 19, 2023

Conversation

dani-garcia
Copy link
Member

Type of change

- [ ] Bug fix
- [ ] New feature development
- [x] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

Split EncString into an asymmetric variant. I've left the symmetric variant with the name EncString as it's the most common variant to avoid major changes.

This is somewhat related to #447 and should be merged first so that PR can take advantage of the symm/asymm separation.

I've moved the EncString files into their own folder to organize the crypto directory a bit better too.

@dani-garcia dani-garcia requested a review from Hinton December 18, 2023 20:08
Copy link

codecov bot commented Dec 18, 2023

Codecov Report

Attention: 58 lines in your changes are missing coverage. Please review.

Comparison is base (9d4ec76) 44.81% compared to head (b67665c) 45.21%.
Report is 1 commits behind head on main.

Files Patch % Lines
...ates/bitwarden/src/crypto/enc_string/asymmetric.rs 75.75% 32 Missing ⚠️
crates/bitwarden/src/crypto/enc_string/mod.rs 68.88% 14 Missing ⚠️
crates/bitwarden/src/uniffi_support.rs 0.00% 7 Missing ⚠️
crates/bitwarden/src/client/encryption_settings.rs 0.00% 2 Missing ⚠️
...rates/bitwarden/src/crypto/enc_string/symmetric.rs 60.00% 2 Missing ⚠️
crates/bitwarden/src/client/client.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #448      +/-   ##
==========================================
+ Coverage   44.81%   45.21%   +0.39%     
==========================================
  Files         151      153       +2     
  Lines        6881     6918      +37     
==========================================
+ Hits         3084     3128      +44     
+ Misses       3797     3790       -7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@Hinton Hinton 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, some thoughts.

I think we should rename EncString to SymcEncString. We should also standardize on our naming conventions, should SymmetricCryptoKey be SymCryptokey (separate PR).

I also wonder if we should keep EncString as an Enum around Sym and Assym.

crates/bitwarden/src/crypto/enc_string/asymmetric.rs Outdated Show resolved Hide resolved
crates/bitwarden/src/crypto/enc_string/asymmetric.rs Outdated Show resolved Hide resolved
crates/bitwarden/src/crypto/enc_string/asymmetric.rs Outdated Show resolved Hide resolved
@dani-garcia
Copy link
Member Author

I chose not to rename EncString to SymmEncString mostly because it's used everywhere, and because it's 90% of our crypto operations, so in a way it's the default, and you have to go out of your way if you want to use asymmetric encryption.

About having an enum encompassing both EncString types, should be easy enough to do in the future but at the moment it doesn't look like we have a need for it. Also I'd be worried that it would give people the idea that it's the main one they would need to use, while we should prefer the use of the compile time explicit Symm/AsymmEncString as much as possible.

The naming convention in my head is:
EncString / SymmCryptoKey
AsymmEncString / AsymmCryptoKey

AnyEncString <- For the enum containing both variants, makes it more explicit that it's a runtime selection

EncString would break the convention of the other names, but I think it's going the far more common type, so it makes sense to me for it to be shorter. Plus we're going to enforce a to compile time that you can only use a symmetric key with a symmetric EncString, so it shouldn't cause accidental misuse.

For the rest, they would see less use in most common use cases, so I'm fine with going with the Symm abreviation or the full Symmetric even.

@dani-garcia dani-garcia merged commit 1067d35 into main Dec 19, 2023
42 checks passed
@dani-garcia dani-garcia deleted the ps/asymm-enc-key branch December 19, 2023 15:04
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