[PM-26354] Add methods to un-/wrap asymmetric keys in keystore context (1/3) #485
Conversation
|
Great job! No new security vulnerabilities introduced in this pull request |
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #485 +/- ##
==========================================
+ Coverage 78.15% 78.25% +0.10%
==========================================
Files 283 283
Lines 27611 27713 +102
==========================================
+ Hits 21579 21688 +109
+ Misses 6032 6025 -7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Changes look mostly good, but I don't think wrapping public keys should be a supported public API, and other teams (aside from what's necessary for the rotateable keyset) should not use it.
Any thoughts on moving the rotateable key set to the crypto crate entirely? We probably don't even need to implement things on the key context then and can go with the keys directly as they are ephemeral for interacting with the key set anyways and shouldn't be held in the key context longer than needed.
If that's not possible I'm OK with making it depracated with the deprecation note stating it's meant for use with the rotateable key set only.
| /// * `wrapping_key` - The key id used to decrypt the `wrapped_key`. It must already exist in | ||
| /// the context | ||
| /// * `wrapped_key` - The key to decrypt | ||
| pub fn unwrap_public_key( |
There was a problem hiding this comment.
Imo this is not a public API that we want to offer to teams. We should either deprecate it with a note that this is only to be used for rotateable key sets, or we should move the rotateable keyset in the follow-up PR into the crypto crate and remove this method (or make it pub(crate).
The same applies for wrap_public_key.
There was a problem hiding this comment.
We probably don't even need to implement things on the key context then and can go with the keys directly as they are ephemeral for interacting with the key set anyways and shouldn't be held in the key context longer than needed.
I had a question about that: does the context received from store.context() live as long as the store or as long as the reference that is returned?
I don't think wrapping public keys should be a supported public API, and other teams (aside from what's necessary for the rotateable keyset) should not use it.
Any thoughts on moving the rotateable key set to the crypto crate entirely?
That's fine by me! I wasn't sure about where ownership between bitwarden-crypto and bitwarden_core::key_management lies, so I put the methods closest to where they would be used. I initially implemented the rotateable key set without the keystore, but I had to use a bunch of things marked as deprecated. Are those just intended to mark dangerous/internal-but-has-to-be-pub methods?
|
In my perspective KM owns the public interface of the keystore, please have a discussion with KM what level of support is required here. And if it can be made generic and in line with existing and future requirements. Also, splitting this up into 3 PRs does not make the review process easier IMHO, since you need all three PRs to understand the scope. |
Sorry about that. I agree this warrants further discussion; I'll mark this as a draft until we've talked further.
Yeah, I see that now. I can drop these and merge them into a single PR after discussion with the KM team. |
🎟️ Tracking
PM-26354
📔 Objective
In order to use support PRF keys in the mobile clients, we need to generate rotateable keysets in . This adds the necessary methods to the keystore needed for creating the rotateable keysets.
⏰ 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