feat(crypto): Support storing the dehydrated device pickle key #4383
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4383 +/- ##
==========================================
+ Coverage 85.27% 85.30% +0.02%
==========================================
Files 282 282
Lines 31207 31257 +50
==========================================
+ Hits 26613 26663 +50
Misses 4594 4594 ☔ View full report in Codecov by Sentry. |
BillCarsonFr
force-pushed
the
valere/cache_dehydration_pickle_key
branch
2 times, most recently
from
821df3d to
3162d33
Compare
BillCarsonFr
requested review from
bnjbvr and
poljar
and removed request for
a team
| @@ -846,6 +852,11 @@ impl CryptoStore for SqliteCryptoStore { | |||
| txn.set_kv("backup_version_v1", &serialized_backup_version)?; | |||
| } | |||
|
|
|||
| if let Some(pickle_key) = &changes.dehydrated_device_pickle_key { | |||
There was a problem hiding this comment.
Same as for the indexedDB implementation, docs and implementation don't match.
| async fn delete_dehydrated_device_pickle_key(&self) -> Result<()> { | ||
| let mut lock = self.dehydrated_device_pickle_key.write().await; | ||
| *lock = None; | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Maybe I'm missing something, but is this method used anywhere? It's part of the trait, but It's not exposed to users nor do we internally delete the key anywhere.
Would it make sense to delete it automagically when we create a new dehydrated device?
There was a problem hiding this comment.
Exposed now. Should be used by apps.
It could make sense if we changed a bit the API, currently create does not need the pickle key, it's upload_keys that use it. Not sure how it would work. Need to talk a bit with @uhoreg to see if can have a better integrated usage of pickle key handling
| key.copy_from_slice(pickle_key); | ||
|
|
||
| // safe to unwrap because we checked the size | ||
| let raw_bytes: [u8; 32] = pickle_key.try_into().unwrap(); |
There was a problem hiding this comment.
I would leave the copy from slice, try_into() moves self and moves are copies in Rust. Now here it's fine because you move a reference so we only copy the reference instead of the whole byte array.
But we always handle secret key material using copy_from_slice() and there it's obvious where the copy is made. The panic/unwrap is then also implicit and it's understood that you need a size check before.
BillCarsonFr
force-pushed
the
valere/cache_dehydration_pickle_key
branch
from
604e379 to
2b39476
Compare
Fixes #4186
No need for migration, the pickle key is stored in the generic Key/Value store.
Signed-off-by: