[PM-22845] Add account key rotation #313
Conversation
|
Great job, no security vulnerabilities found in this Pull Request |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## km/cose-content-format #313 +/- ##
==========================================================
+ Coverage 71.73% 71.84% +0.11%
==========================================================
Files 239 227 -12
Lines 19342 18851 -491
==========================================================
- Hits 13875 13544 -331
+ Misses 5467 5307 -160 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
quexten
force-pushed
the
km/cose-key-rotation
branch
from
f72b76e to
4becfdd
Compare
| } | ||
|
|
||
| /// Re-encrypts the user's keys with the provided symmetric key for a v2 user. | ||
| pub fn get_v2_rotated_account_keys<Ids: crate::KeyIds>( |
There was a problem hiding this comment.
question: Key rotation feels very application bound and not like a low level cryptographic primitive? I think this is further enhanced by bitwarden-core just acting as a wrapper around this function.
I presume this was done to avoid making get_asymetric_key and `get_signing_key public?
There was a problem hiding this comment.
Yes;
As far as I understand we do not want any further new uses that expose keys out of bitwarden-crypto, so this had to go into the crypto crate to achieve that. Is there another pattern we can use here? (Maybe we do allow (non-deprecated) getting wrapped(encrypted) keys out of a context?)
There was a problem hiding this comment.
I think it's important to consider the public API of crates. In this case we have a get_v2_rotated_account_keys which anyone can call for any reason. It provides a way into the "locked" down areas of the Context.
To start with we can move rotate into context that gets rid of the pub(crate). Prefixing it with dangerous similar to other operations we don't want to maintain long term also seems reasonable? Ideally we'd find a way to get rid of the response struct but we'd essentially just be duplicating it ... and keeping it where it is seems easier.
Having dangerous wrappers to extract keys until we can lift in the whole key rotation also seems fine.
|
How do we envision the final key rotation to behave? Context generates new keys for you, it stores them in context and exposes encrypted and signed variants? Currently we'd need |
Does final refer to within the context of the current changeset, or in the long run? In the long run, with state living in the sdk, calling rotate would:
In the short term, what core does right now, is only done in So, currently the rotation is intended to be used with a fresh, locally generated SDK instance for the current set of keys. Based on this, the sdk returns a rotated set of keys. The SDK only ever touches the local context, and does not modify key-context outside of local scope. Then, the client takes the result, sends it to the server and logs out: https://github.com/bitwarden/clients/blob/91538819767e46f4b6045d8ff115ddda1aff1843/apps/web/src/app/key-management/key-rotation/user-key-rotation.service.ts#L309 |
|
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-22845
📔 Objective
The decrypted signing key is never available to Typescript, and we need key-rotation to re-encrypt the signing key. This PR implements a part of key-rotation in the crypto crate - specifically re-encrypting the private and signing key.
Long-term, the key-rotation could should be ported at higher levels to the crypto crate.
⏰ Reminders before review
team
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmedissue and could potentially benefit from discussion
:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes