[PM-27232] Implement Registration for TDE Users #596
Conversation
|
Great job! No new security vulnerabilities introduced in this pull request |
|
🔍 SDK Breaking Change Detection ResultsSDK Version:
Breaking change detection completed. View SDK workflow |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #596 +/- ##
==========================================
- Coverage 78.67% 78.51% -0.17%
==========================================
Files 283 283
Lines 29285 29345 +60
==========================================
- Hits 23041 23039 -2
- Misses 6244 6306 +62 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
quexten
changed the title
quexten
force-pushed
the
km/tde-registration
branch
from
7e40878 to
7fe5d69
Compare
quexten
requested review from
JaredSnider-Bitwarden,
Thomas-Avery and
djsmith85
This comment was marked as resolved.
This comment was marked as resolved.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Thomas-Avery
left a comment
Thomas-Avery
left a comment
There was a problem hiding this comment.
Looking good. A few things to take a look at and some questions from my side.
Also I think claude is wanting to have a chat on this one 😆
| org_id: String, | ||
| org_public_key: B64, | ||
| // Note: Ideally these would be set for the register client, however no such functionality | ||
| // exists at the moment | ||
| user_id: String, | ||
| device_id: String, | ||
| trust_device: bool, |
There was a problem hiding this comment.
❓
For these ID parameters can we use the OrganizationId UserId etc., from crates/bitwarden-core/src/ids.rs, as the input types? Or is there some limitation for those?
| &self, | ||
| org_id: String, | ||
| org_public_key: B64, | ||
| // Note: Ideally these would be set for the register client, however no such functionality | ||
| // exists at the moment | ||
| user_id: String, | ||
| device_id: String, | ||
| trust_device: bool, |
There was a problem hiding this comment.
I agree with Jared on this, mostly just a style/preference thing having a bunch of primitives is hard to keep track of when reading the method.
Vs just seeing request.<some_input_name> lets me know right away it's coming from the request input.
| // Note: This property is deprecated and will be removed | ||
| public_key: account_cryptographic_state_request | ||
| .account_public_key | ||
| .ok_or(UserRegistrationError::Crypto)?, | ||
| // Note: This property is deprecated and will be removed |
There was a problem hiding this comment.
For these notes do we have a ticket tracking that, that we can reference here in the comments?
| pub device_key: String, | ||
| /// The decrypted user key. This can be used to get the consuming client to an unlocked state. | ||
| pub user_key: B64, |
There was a problem hiding this comment.
❓ Why do we use String for device_key and B64 for user_key?
Since they're both symmetric crypto keys I would expect the same return type.
| let mut ctx = self.client.internal.get_key_store().context_mut(); | ||
| let (user_key, wrapped_state) = | ||
| WrappedAccountCryptographicState::make(&mut ctx, user_id) | ||
| .map_err(MakeKeysError::AccountCryptographyInitialization)?; | ||
| #[expect(deprecated)] | ||
| let user_key = ctx.dangerous_get_symmetric_key(user_key)?; | ||
|
|
||
| // TDE unlock method | ||
| let device_key = DeviceKey::trust_device(user_key)?; | ||
|
|
||
| // Account recovery enrollment | ||
| let public_key = | ||
| AsymmetricPublicCryptoKey::from_der(&SpkiPublicKeyBytes::from(&org_public_key)) | ||
| .map_err(MakeKeysError::Crypto)?; | ||
| let admin_reset = UnsignedSharedKey::encapsulate_key_unsigned(user_key, &public_key) | ||
| .map_err(MakeKeysError::Crypto)?; | ||
|
|
||
| let store = KeyStore::default(); | ||
| let mut ctx = store.context_mut(); | ||
| let user_key_id = ctx.add_local_symmetric_key(user_key.to_owned()); | ||
| let security_state = RwLock::new(None); | ||
| wrapped_state | ||
| .set_to_context(&security_state, user_key_id, &store, ctx) | ||
| .map_err(MakeKeysError::AccountCryptographyInitialization)?; | ||
|
|
||
| let cryptography_state_request_model = wrapped_state | ||
| .to_request_model(&store) | ||
| .map_err(|_| MakeKeysError::RequestModelCreation)?; | ||
|
|
||
| Ok(( | ||
| wrapped_state, | ||
| user_key.to_owned(), | ||
| cryptography_state_request_model, | ||
| device_key, | ||
| admin_reset, | ||
| )) |
There was a problem hiding this comment.
| let mut ctx = self.client.internal.get_key_store().context_mut(); | |
| let (user_key, wrapped_state) = | |
| WrappedAccountCryptographicState::make(&mut ctx, user_id) | |
| .map_err(MakeKeysError::AccountCryptographyInitialization)?; | |
| #[expect(deprecated)] | |
| let user_key = ctx.dangerous_get_symmetric_key(user_key)?; | |
| // TDE unlock method | |
| let device_key = DeviceKey::trust_device(user_key)?; | |
| // Account recovery enrollment | |
| let public_key = | |
| AsymmetricPublicCryptoKey::from_der(&SpkiPublicKeyBytes::from(&org_public_key)) | |
| .map_err(MakeKeysError::Crypto)?; | |
| let admin_reset = UnsignedSharedKey::encapsulate_key_unsigned(user_key, &public_key) | |
| .map_err(MakeKeysError::Crypto)?; | |
| let store = KeyStore::default(); | |
| let mut ctx = store.context_mut(); | |
| let user_key_id = ctx.add_local_symmetric_key(user_key.to_owned()); | |
| let security_state = RwLock::new(None); | |
| wrapped_state | |
| .set_to_context(&security_state, user_key_id, &store, ctx) | |
| .map_err(MakeKeysError::AccountCryptographyInitialization)?; | |
| let cryptography_state_request_model = wrapped_state | |
| .to_request_model(&store) | |
| .map_err(|_| MakeKeysError::RequestModelCreation)?; | |
| Ok(( | |
| wrapped_state, | |
| user_key.to_owned(), | |
| cryptography_state_request_model, | |
| device_key, | |
| admin_reset, | |
| )) | |
| let mut ctx = self.client.internal.get_key_store().context_mut(); | |
| let (user_key_id, wrapped_state) = | |
| WrappedAccountCryptographicState::make(&mut ctx, user_id) | |
| .map_err(MakeKeysError::AccountCryptographyInitialization)?; | |
| #[expect(deprecated)] | |
| let user_key = ctx.dangerous_get_symmetric_key(user_key_id)?; | |
| // TDE unlock method | |
| let device_key = DeviceKey::trust_device(user_key)?; | |
| // Account recovery enrollment | |
| let public_key = | |
| AsymmetricPublicCryptoKey::from_der(&SpkiPublicKeyBytes::from(&org_public_key)) | |
| .map_err(MakeKeysError::Crypto)?; | |
| let admin_reset = UnsignedSharedKey::encapsulate_key_unsigned(user_key, &public_key) | |
| .map_err(MakeKeysError::Crypto)?; | |
| let cryptography_state_request_model = wrapped_state | |
| .to_request_model(self.client.internal.get_key_store()) | |
| .map_err(|_| MakeKeysError::RequestModelCreation)?; | |
| Ok(( | |
| wrapped_state, | |
| user_key.to_owned(), | |
| cryptography_state_request_model, | |
| device_key, | |
| admin_reset, | |
| )) |
💭 It would seem like we would want to continue using the ctx and key store from the client?
Maybe I'm missing something important or a side effect making the other key store is desired for. If so could we add a comment about what that is.
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-27232
bitwarden/server#6671
📔 Objective
Implements registration for TDE users. This uses v2 encryption.
Note
The API changes here are not final and need an update to the automatic API bindings, after the corresponding server change is merged. This PR will only be merged once the binding changes are already in
mainand removed from this PR.🚨 Breaking Changes
⏰ 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