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

[PM-115] Cipher key encryption update #2421

Merged
merged 31 commits into from
Sep 28, 2023

Conversation

fedemkr
Copy link
Member

@fedemkr fedemkr commented Mar 20, 2023

Type of change

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

Objective

The cipher will now have its own Key to encrypt/decrypt its content which will be encrypted using the master password or organization key.

This needs server changes to work and clients changes to have the key assigned (given that for now the mobile client won't create the cipher key):

Code changes

  • Cipher: Added new properties for the key. Added logic to decrypt using the new Key if available, passing such key to the different item types. And updated it to not use hardcoded strings and use nameof().

  • CipherService: Updated logic to encrypt the Cipher using the new Key.
    Note: for now the logic of the new Key gets applied only if the Key already exists on the Cipher. The creation of such Key locally will be enabled on a later release to avoid conflicts between clients.

  • CipherData: Added new properties for the key.

  • CipherRequest, CipherResponse, CipherView: Added new properties for the key.

  • Attachment: Updated it to not use hardcoded strings and use nameof() and also added the key as a parameter to support the new approach.

  • Card, Field, Identity, Login, LoginUri, PasswordHistory, SecureNote: Added the key as a parameter to support the new approach.

Before you submit

  • Please check for formatting errors (dotnet format --verify-no-changes) (required)
  • Please add unit tests where it makes sense to do so (encouraged but not required)
  • If this change requires a documentation update - notify the documentation team
  • If this change has particular deployment requirements - notify the DevOps team

mpbw2
mpbw2 previously approved these changes Mar 20, 2023
Copy link
Contributor

@mpbw2 mpbw2 left a comment

Choose a reason for hiding this comment

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

How is performance when flinging through the cipher list?

@fedemkr
Copy link
Member Author

fedemkr commented Mar 20, 2023

As far as I see, the entire cipher list gets decrypted before shown to the user so the performance while flinging is the same as before. But yes, we should have a little bit more overhead while decrypting the whole vault ciphers.

@fedemkr fedemkr added the hold do not merge yet label Mar 20, 2023
@mpbw2
Copy link
Contributor

mpbw2 commented Mar 21, 2023

Ah ok, I was thinking this could provide some performance insight to adapter-driven content if/when we take that step.

@fedemkr
Copy link
Member Author

fedemkr commented Mar 21, 2023

I think we can measure that and see how it would impact in such situation. However, if we take that route we should probably cache more things in order to have the smoothest UI/UX and refactor the code accordingly. I believe that if we paginate or lazy decrypt the ciphers as they appear it would have a really fast load time of the page but we need to think it carefully a preload/predecrypt ciphers so that flinging through the list doesn't get much affected by it

fedemkr added 2 commits May 23, 2023 17:48
…yption (#2463)

* PM-1690 added minimum server version restriction to cipher key encryption and also change the force key rotation flag

* PM-1690 Updated min server version for new cipher encryption key and fixed configService registration

* PM-1690 removed forcekeyrotation
@fedemkr fedemkr requested a review from mpbw2 May 29, 2023 15:36
mpbw2
mpbw2 previously approved these changes May 30, 2023
…esting (this change should be reseted eventually)
@fedemkr fedemkr requested a review from mpbw2 June 5, 2023 14:57
mpbw2
mpbw2 previously approved these changes Jun 5, 2023
@fedemkr fedemkr requested a review from mpbw2 June 28, 2023 13:48
mpbw2
mpbw2 previously approved these changes Jun 28, 2023
# Conflicts:
#	src/Core/Constants.cs
#	src/Core/Models/Response/CipherResponse.cs
@bitwarden-bot
Copy link

bitwarden-bot commented Jul 27, 2023

Logo
Checkmarx One – Scan Summary & Details055f33bf-7ed7-4c7b-adf6-543b892a415b

No New Or Fixed Issues Found

@fedemkr fedemkr removed the hold do not merge yet label Aug 31, 2023
@fedemkr fedemkr requested a review from mpbw2 August 31, 2023 14:17
mpbw2
mpbw2 previously approved these changes Aug 31, 2023
Updated minimum encryption server version to 2023.9.0 so QA can test its behavior
@fedemkr fedemkr requested a review from mpbw2 September 27, 2023 19:35
@fedemkr fedemkr removed the needs-qa label Sep 27, 2023
@fedemkr fedemkr merged commit 3cdf5cc into master Sep 28, 2023
@fedemkr fedemkr deleted the feature/PM-115-encryption-update-new-model branch September 28, 2023 13:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants