-
Notifications
You must be signed in to change notification settings - Fork 24
[PM-27231] Account registration v2 for key connector #611
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,7 +5,23 @@ | |
| //! authentication method such as SSO or master password, and a decryption method such as | ||
| //! key-connector, TDE, or master password. | ||
|
|
||
| use bitwarden_core::Client; | ||
| use std::str::FromStr; | ||
|
|
||
| use bitwarden_api_api::models::SetKeyConnectorKeyRequestModel; | ||
| use bitwarden_core::{ | ||
| Client, UserId, | ||
| key_management::{ | ||
| AccountCryptographyMakeKeysError, KeyConnectorApiError, | ||
| account_cryptographic_state::WrappedAccountCryptographicState, | ||
| key_connector_api_post_or_put_key_connector_key, | ||
| }, | ||
| }; | ||
| use bitwarden_crypto::EncString; | ||
| use bitwarden_encoding::B64; | ||
| use bitwarden_error::bitwarden_error; | ||
| use serde_bytes::ByteBuf; | ||
| use thiserror::Error; | ||
| use tracing::info; | ||
| #[cfg(feature = "wasm")] | ||
| use wasm_bindgen::prelude::*; | ||
|
|
||
|
|
@@ -33,4 +49,99 @@ impl RegistrationClient { | |
| let api_client = &client.get_api_configurations().await.api_client; | ||
| // Do API request here. It will be authenticated using the client's tokens. | ||
| } | ||
|
|
||
| /// Initializes a new cryptographic state for a user and posts it to the server; enrolls the | ||
| /// user to key connector unlock. | ||
| pub async fn post_keys_for_key_connector_registration( | ||
| &self, | ||
| key_connector_url: String, | ||
| org_id: String, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Relaying feedback, this should use OrganizationId, UserId instead of the string types. |
||
| user_id: String, | ||
| ) -> Result<KeyConnectorRegistrationResult, UserRegistrationError> { | ||
| let client = &self.client.internal; | ||
| let api_client = &client.get_api_configurations().await.api_client; | ||
| let user_id = | ||
| UserId::from_str(user_id.as_str()).map_err(|_| UserRegistrationError::Serialization)?; | ||
|
|
||
| // First call crypto API to get all keys | ||
| info!("Initializing account cryptography"); | ||
| let ( | ||
| cryptography_state, | ||
| wrapped_user_key, | ||
| user_key, | ||
| account_cryptographic_state_request, | ||
| key_connector_key, | ||
| ) = self | ||
| .client | ||
| .crypto() | ||
| .make_user_key_connector_registration(user_id) | ||
| .map_err(UserRegistrationError::AccountCryptographyMakeKeys)?; | ||
|
|
||
| info!("Posting key connector key to key connector server"); | ||
| key_connector_api_post_or_put_key_connector_key( | ||
| &self.client, | ||
| key_connector_url.as_str(), | ||
| &key_connector_key, | ||
| ) | ||
| .await | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Relaying feedback; We should add tracing for the errors here. We don't want to expand the returned error type, but log that the api request went wrong (and possibly how if it doesn't contain sensitive data). |
||
| .map_err(UserRegistrationError::KeyConnectorApi)?; | ||
|
|
||
| info!("Posting user account cryptographic state to server"); | ||
| let request = SetKeyConnectorKeyRequestModel { | ||
| key_connector_key_wrapped_user_key: Some(wrapped_user_key.to_string()), | ||
| account_keys: Some(Box::new(account_cryptographic_state_request)), | ||
| ..SetKeyConnectorKeyRequestModel::new(org_id) | ||
| }; | ||
| api_client | ||
| .accounts_key_management_api() | ||
| .post_set_key_connector_key(Some(request)) | ||
| .await | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here for tracing the error |
||
| .map_err(|e| UserRegistrationError::Api(e.into()))?; | ||
|
|
||
| info!("User initialized!"); | ||
| // Note: This passing out of state and keys is temporary. Once SDK state management is more | ||
| // mature, the account cryptographic state and keys should be set directly here. | ||
| Ok(KeyConnectorRegistrationResult { | ||
| account_cryptographic_state: cryptography_state, | ||
| key_connector_key: key_connector_key.to_base64(), | ||
| key_connector_key_wrapped_user_key: wrapped_user_key, | ||
| user_key: user_key.to_encoded().to_vec().into(), | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| /// Result of Key Connector registration process. | ||
| #[cfg_attr( | ||
| feature = "wasm", | ||
| derive(tsify::Tsify), | ||
| tsify(into_wasm_abi, from_wasm_abi) | ||
| )] | ||
| #[derive(serde::Serialize, serde::Deserialize, Clone, Debug)] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should derive UNIFFI, however, please note that you have to merge in latest main, because auth fixed some stuff for UNIFFI in the meantime. |
||
| pub struct KeyConnectorRegistrationResult { | ||
| /// The account cryptographic state of the user. | ||
| pub account_cryptographic_state: WrappedAccountCryptographicState, | ||
| /// The key connector key used for unlocking. | ||
| pub key_connector_key: B64, | ||
| /// The encrypted user key, wrapped with the key connector key. | ||
| pub key_connector_key_wrapped_user_key: EncString, | ||
| /// The decrypted user key. This can be used to get the consuming client to an unlocked state. | ||
| pub user_key: ByteBuf, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be updated to B64 (the TDE PR does this too), since both user_key and key_connector_key are keys. Also, ByteBuf is not nicely supported by uniffi |
||
| } | ||
|
|
||
| /// Errors that can occur during user registration. | ||
| #[derive(Debug, Error)] | ||
| #[bitwarden_error(flat)] | ||
| pub enum UserRegistrationError { | ||
| /// Key Connector API call failed. | ||
| #[error(transparent)] | ||
| KeyConnectorApi(#[from] KeyConnectorApiError), | ||
| /// API call failed. | ||
| #[error(transparent)] | ||
| Api(#[from] bitwarden_core::ApiError), | ||
| /// Account cryptography initialization failed. | ||
| #[error(transparent)] | ||
| AccountCryptographyMakeKeys(#[from] AccountCryptographyMakeKeysError), | ||
| /// Serialization or deserialization error | ||
| #[error("Serialization error")] | ||
| Serialization, | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Relaying feedback I got on my PR here, we probably want unit tests on this. Vault has some examples for this, but it generally involves moving the impl into a separate function that you pass in the "api client" dependency and other parameters to, then the test can replace that with a mocked api client.