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-1339] Allow Rotating Device Keys #3096

Merged

Conversation

justindbaur
Copy link
Member

@justindbaur justindbaur commented Jul 13, 2023

Type of change

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

Objective

Add server side components for facilitating the rotation of the current devices keys alongside any other chosen device keys. This makes it so that when a user rotates their user key they are not locked out of all their devices.

Related clients PR: bitwarden/clients#5806

Code changes

  • UpdateDevicesTrustRequestModel.cs: A model that requires the inclusion of at least new keys for the current device and optionally allows n number of other devices the user wants rotated.
  • DevicesController.cs: Added two new endpoints for retrieving the rotation specific keys for a device by it's ID behind a user verification check and for updating the trust of the users devices, also behind a user verification check.
  • DeviceResponseModel.cs: Updated default device model to only respond that a device is trusted or not, and to not include the keys. Although they are encrypted with a key owned by the user, they should not be needlessly shared.
  • DeviceService.cs: Added method for facilitating the rotation of all devices keys requested by the user.
  • DeviceServiceTests.cs: Added tests asserting expected behavior from rotating trust.

Before you submit

  • Please check for formatting errors (dotnet format --verify-no-changes) (required)
  • If making database changes - make sure you also update Entity Framework queries and/or migrations
  • 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

- Add endpoint for getting keys relating to rotation
- Add endpoint for rotating your current device
- In the same endpoint allow a list of other devices to rotate
@justindbaur justindbaur marked this pull request as ready for review July 13, 2023 02:01
@justindbaur justindbaur requested a review from a team as a code owner July 13, 2023 02:01
Copy link
Contributor

@JaredSnider-Bitwarden JaredSnider-Bitwarden left a comment

Choose a reason for hiding this comment

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

Awesome work on this! Thank you very much for the thoughtful design of the new DevicesController endpoints!

Just a few minor questions on the testing of the UpdateDevicesTrustAsync method.

src/Core/Services/Implementations/DeviceService.cs Outdated Show resolved Hide resolved
test/Core.Test/Services/DeviceServiceTests.cs Outdated Show resolved Hide resolved
test/Core.Test/Services/DeviceServiceTests.cs Show resolved Hide resolved
test/Core.Test/Services/DeviceServiceTests.cs Show resolved Hide resolved
src/Api/Controllers/DevicesController.cs Show resolved Hide resolved
Co-authored-by: Jared Snider <[email protected]>
Copy link
Contributor

@JaredSnider-Bitwarden JaredSnider-Bitwarden left a comment

Choose a reason for hiding this comment

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

LGTM!

@justindbaur justindbaur merged commit eb539e8 into feature/trusted-device-encryption Jul 20, 2023
41 checks passed
@justindbaur justindbaur deleted the auth/pm-1339/rotate-device-keys branch July 20, 2023 11:07
jlf0dev pushed a commit that referenced this pull request Aug 17, 2023
* [PM-1203] feat: allow verification for all passwordless accounts (#3038)

* [PM-1033] Org invite user creation flow 1 (#3028)

* [PM-1033] feat: remove user verification from password enrollment

* [PM-1033] feat: auto accept invitation when enrolling into password reset

* [PM-1033] fix: controller tests

* [PM-1033] refactor: `UpdateUserResetPasswordEnrollmentCommand`

* [PM-1033] refactor(wip): make `AcceptUserCommand`

* Revert "[PM-1033] refactor(wip): make `AcceptUserCommand`"

This reverts commit dc1319e.

* Revert "[PM-1033] refactor: `UpdateUserResetPasswordEnrollmentCommand`"

This reverts commit 43df689.

* [PM-1033] refactor: move invite accept to controller

This avoids creating yet another method that depends on having `IUserService` passed in as a parameter

* [PM-1033] fix: add missing changes

* [PM-1381] Add Trusted Device Keys to Auth Response (#3066)

* Return Keys for Trusted Device

- Check whether the current logging in device is trusted
- Return their keys on successful login

* Formatting

* Address PR Feedback

* Add Remarks Comment

* [PM-1338] `AuthRequest` Event Logs (#3046)

* Update AuthRequestController

- Only allow AdminApproval Requests to be created from authed endpoint
- Add endpoint that has authentication to be able to create admin approval

* Add PasswordlessAuthSettings

- Add settings for customizing expiration times

* Add new EventTypes

* Add Logic for AdminApproval Type

- Add logic for validating AdminApproval expiration
- Add event logging for Approval/Disapproval of AdminApproval
- Add logic for creating AdminApproval types

* Add Test Helpers

- Change BitAutoData to allow you to use string representations of common types.

* Add/Update AuthRequestService Tests

* Run Formatting

* Switch to 7 Days

* Add Test Covering ResponseDate Being Set

* Address PR Feedback

- Create helper for checking if date is expired
- Move validation logic into smaller methods

* Switch to User Event Type

- Make RequestDeviceApproval user type
- User types will log for each org user is in

* [PM-2998] Move Approving Device Check (#3101)

* Move Check for Approving Devices

- Exclude currently logging in device
- Remove old way of checking
- Add tests asserting behavior

* Update DeviceType list

* Update Naming & Address PR Feedback

* Fix Tests

* Address PR Feedback

* Formatting

* Now Fully Update Naming?

* Feature/auth/pm 2759/add can reset password to user decryption options (#3113)

* PM-2759 - BaseRequestValidator.cs - CreateUserDecryptionOptionsAsync - Add new hasManageResetPasswordPermission for post SSO redirect logic required on client.

* PM-2759 - Update IdentityServerSsoTests.cs to all pass based on the addition of HasManageResetPasswordPermission to TrustedDeviceUserDecryptionOption

* IdentityServerSsoTests.cs - fix typo in test name:  LoggingApproval --> LoginApproval

* PM1259 - Add test case for verifying that TrustedDeviceOption.hasManageResetPasswordPermission is set properly based on user permission

* dotnet format run

* Feature/auth/pm 2759/add can reset password to user decryption options fix jit users (#3120)

* PM-2759 - IdentityServer - CreateUserDecryptionOptionsAsync - hasManageResetPasswordPermission set logic was broken for JIT provisioned users as I assumed we would always have a list of at least 1 org during the SSO process. Added TODO for future test addition but getting this out there now as QA is blocked by being unable to create JIT provisioned users.

* dotnet format

* Tiny tweak

* [PM-1339] Allow Rotating Device Keys (#3096)

* Allow Rotation of Trusted Device Keys

- Add endpoint for getting keys relating to rotation
- Add endpoint for rotating your current device
- In the same endpoint allow a list of other devices to rotate

* Formatting

* Use Extension Method

* Add Tests from PR

Co-authored-by: Jared Snider <[email protected]>

---------

Co-authored-by: Jared Snider <[email protected]>

* Check the user directly if they have the ResetPasswordKey (#3153)

* PM-3327 - UpdateKeyAsync must exempt the currently calling device from the logout notification in order to prevent prematurely logging the user out before the client side key rotation process can complete. The calling device will log itself out once it is done. (#3170)

* Allow OTP Requests When Users Are On TDE (#3184)

* [PM-3356][PM-3292] Allow OTP For All (#3188)

* Allow OTP For All

- On a trusted device isn't a good check because a user might be using a trusted device locally but not trusted it long term
- The logic wasn't working for KC users anyways

* Remove Old Comment

* [AC-1601] Added RequireSso policy as a dependency of TDE (#3209)

* Added RequireSso policy as a dependency of TDE.

* Added test for RequireSso for TDE.

* Added save.

* Fixed policy name.

---------

Co-authored-by: Andreas Coroiu <[email protected]>
Co-authored-by: Justin Baur <[email protected]>
Co-authored-by: Vincent Salucci <[email protected]>
Co-authored-by: Jared Snider <[email protected]>
Co-authored-by: Jared Snider <[email protected]>
trmartin4 added a commit that referenced this pull request Aug 17, 2023
* [PM-1203] feat: allow verification for all passwordless accounts (#3038)

* [PM-1033] Org invite user creation flow 1 (#3028)

* [PM-1033] feat: remove user verification from password enrollment

* [PM-1033] feat: auto accept invitation when enrolling into password reset

* [PM-1033] fix: controller tests

* [PM-1033] refactor: `UpdateUserResetPasswordEnrollmentCommand`

* [PM-1033] refactor(wip): make `AcceptUserCommand`

* Revert "[PM-1033] refactor(wip): make `AcceptUserCommand`"

This reverts commit dc1319e.

* Revert "[PM-1033] refactor: `UpdateUserResetPasswordEnrollmentCommand`"

This reverts commit 43df689.

* [PM-1033] refactor: move invite accept to controller

This avoids creating yet another method that depends on having `IUserService` passed in as a parameter

* [PM-1033] fix: add missing changes

* [PM-1381] Add Trusted Device Keys to Auth Response (#3066)

* Return Keys for Trusted Device

- Check whether the current logging in device is trusted
- Return their keys on successful login

* Formatting

* Address PR Feedback

* Add Remarks Comment

* [PM-1338] `AuthRequest` Event Logs (#3046)

* Update AuthRequestController

- Only allow AdminApproval Requests to be created from authed endpoint
- Add endpoint that has authentication to be able to create admin approval

* Add PasswordlessAuthSettings

- Add settings for customizing expiration times

* Add new EventTypes

* Add Logic for AdminApproval Type

- Add logic for validating AdminApproval expiration
- Add event logging for Approval/Disapproval of AdminApproval
- Add logic for creating AdminApproval types

* Add Test Helpers

- Change BitAutoData to allow you to use string representations of common types.

* Add/Update AuthRequestService Tests

* Run Formatting

* Switch to 7 Days

* Add Test Covering ResponseDate Being Set

* Address PR Feedback

- Create helper for checking if date is expired
- Move validation logic into smaller methods

* Switch to User Event Type

- Make RequestDeviceApproval user type
- User types will log for each org user is in

* [PM-2998] Move Approving Device Check (#3101)

* Move Check for Approving Devices

- Exclude currently logging in device
- Remove old way of checking
- Add tests asserting behavior

* Update DeviceType list

* Update Naming & Address PR Feedback

* Fix Tests

* Address PR Feedback

* Formatting

* Now Fully Update Naming?

* Feature/auth/pm 2759/add can reset password to user decryption options (#3113)

* PM-2759 - BaseRequestValidator.cs - CreateUserDecryptionOptionsAsync - Add new hasManageResetPasswordPermission for post SSO redirect logic required on client.

* PM-2759 - Update IdentityServerSsoTests.cs to all pass based on the addition of HasManageResetPasswordPermission to TrustedDeviceUserDecryptionOption

* IdentityServerSsoTests.cs - fix typo in test name:  LoggingApproval --> LoginApproval

* PM1259 - Add test case for verifying that TrustedDeviceOption.hasManageResetPasswordPermission is set properly based on user permission

* dotnet format run

* Feature/auth/pm 2759/add can reset password to user decryption options fix jit users (#3120)

* PM-2759 - IdentityServer - CreateUserDecryptionOptionsAsync - hasManageResetPasswordPermission set logic was broken for JIT provisioned users as I assumed we would always have a list of at least 1 org during the SSO process. Added TODO for future test addition but getting this out there now as QA is blocked by being unable to create JIT provisioned users.

* dotnet format

* Tiny tweak

* [PM-1339] Allow Rotating Device Keys (#3096)

* Allow Rotation of Trusted Device Keys

- Add endpoint for getting keys relating to rotation
- Add endpoint for rotating your current device
- In the same endpoint allow a list of other devices to rotate

* Formatting

* Use Extension Method

* Add Tests from PR

Co-authored-by: Jared Snider <[email protected]>

---------

Co-authored-by: Jared Snider <[email protected]>

* Check the user directly if they have the ResetPasswordKey (#3153)

* PM-3327 - UpdateKeyAsync must exempt the currently calling device from the logout notification in order to prevent prematurely logging the user out before the client side key rotation process can complete. The calling device will log itself out once it is done. (#3170)

* Allow OTP Requests When Users Are On TDE (#3184)

* [PM-3356][PM-3292] Allow OTP For All (#3188)

* Allow OTP For All

- On a trusted device isn't a good check because a user might be using a trusted device locally but not trusted it long term
- The logic wasn't working for KC users anyways

* Remove Old Comment

* [AC-1601] Added RequireSso policy as a dependency of TDE (#3209)

* Added RequireSso policy as a dependency of TDE.

* Added test for RequireSso for TDE.

* Added save.

* Fixed policy name.

---------

Co-authored-by: Andreas Coroiu <[email protected]>
Co-authored-by: Justin Baur <[email protected]>
Co-authored-by: Vincent Salucci <[email protected]>
Co-authored-by: Jared Snider <[email protected]>
Co-authored-by: Jared Snider <[email protected]>
(cherry picked from commit 1c3afcd)
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