Skip to content

Commit

Permalink
Return a more detailed MissingKey error from get_key() (#900)
Browse files Browse the repository at this point in the history
## 🎟️ Tracking

<!-- Paste the link to the Jira or GitHub issue or otherwise describe /
point to where this change is coming from. -->

## 📔 Objective

This change adds the organization UUID to the `MissingKey` error type,
and changes the `get_key` function to return a Result instead of an
Option to avoid having to add the error all over the place.

Note that some of the `encryption_settings.get_key()` calls were being
mapped to `VaultLocked`, that wasn't correct as having the
`EncryptionSettings` means the vault is unlocked. Those were changed to
`MissingKey(id)` as well.



## 📸 Screenshots

<!-- Required for any UI changes; delete if not applicable. Use fixed
width images for better display. -->

## ⏰ Reminders before review

- Contributor guidelines followed
- All formatters and local linters executed and passed
- Written new unit and / or integration tests where applicable
- Protected functional changes with optionality (feature flags)
- Used internationalization (i18n) for all UI strings
- CI builds passed
- Communicated to DevOps any deployment requirements
- Updated any necessary documentation (Confluence, contributing docs) or
informed the documentation
  team

## 🦮 Reviewer guidelines

<!-- Suggested interactions but feel free to use (or not) as you desire!
-->

- 👍 (`:+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 confirmed
  issue 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
  • Loading branch information
dani-garcia authored Jul 15, 2024
1 parent 335e9d3 commit 69c1735
Show file tree
Hide file tree
Showing 26 changed files with 93 additions and 102 deletions.
4 changes: 2 additions & 2 deletions crates/bitwarden-core/src/auth/auth_request.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use bitwarden_crypto::{
#[cfg(feature = "internal")]
use bitwarden_crypto::{EncString, KeyDecryptable, SymmetricCryptoKey};

use crate::{error::Error, Client, VaultLocked};
use crate::{error::Error, Client};

#[cfg_attr(feature = "uniffi", derive(uniffi::Record))]
pub struct AuthRequestResponse {
Expand Down Expand Up @@ -82,7 +82,7 @@ pub(crate) fn approve_auth_request(
let public_key = AsymmetricPublicCryptoKey::from_der(&STANDARD.decode(public_key)?)?;

let enc = client.internal.get_encryption_settings()?;
let key = enc.get_key(&None).ok_or(VaultLocked)?;
let key = enc.get_key(&None)?;

Ok(AsymmetricEncString::encrypt_rsa2048_oaep_sha1(
&key.to_vec(),
Expand Down
4 changes: 1 addition & 3 deletions crates/bitwarden-core/src/auth/client_auth.rs
Original file line number Diff line number Diff line change
Expand Up @@ -150,11 +150,9 @@ impl<'a> ClientAuth<'a> {

#[cfg(feature = "internal")]
fn trust_device(client: &Client) -> Result<TrustDeviceResponse> {
use crate::VaultLocked;

let enc = client.internal.get_encryption_settings()?;

let user_key = enc.get_key(&None).ok_or(VaultLocked)?;
let user_key = enc.get_key(&None)?;

Ok(DeviceKey::trust_device(user_key)?)
}
Expand Down
9 changes: 2 additions & 7 deletions crates/bitwarden-core/src/auth/password/validate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,6 @@ pub(crate) fn validate_password_user_key(
password: String,
encrypted_user_key: String,
) -> Result<String> {
use crate::VaultLocked;

let login_method = client
.internal
.get_login_method()
Expand All @@ -61,12 +59,9 @@ pub(crate) fn validate_password_user_key(
.decrypt_user_key(encrypted_user_key.parse()?)
.map_err(|_| "wrong password")?;

let enc = client
.internal
.get_encryption_settings()
.map_err(|_| VaultLocked)?;
let enc = client.internal.get_encryption_settings()?;

let existing_key = enc.get_key(&None).ok_or(VaultLocked)?;
let existing_key = enc.get_key(&None)?;

if user_key.to_vec() != existing_key.to_vec() {
return Err("wrong user key".into());
Expand Down
2 changes: 1 addition & 1 deletion crates/bitwarden-core/src/auth/renew.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ pub(crate) async fn renew_token(client: &InternalClient) -> Result<()> {
if let (IdentityTokenResponse::Payload(r), Some(state_file), Ok(enc_settings)) =
(&result, state_file, client.get_encryption_settings())
{
if let Some(enc_key) = enc_settings.get_key(&None) {
if let Ok(enc_key) = enc_settings.get_key(&None) {
let state =
ClientState::new(r.access_token.clone(), enc_key.to_base64());
_ = state::set(state_file, access_token, state);
Expand Down
15 changes: 9 additions & 6 deletions crates/bitwarden-core/src/client/encryption_settings.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use std::collections::HashMap;

use bitwarden_crypto::{AsymmetricCryptoKey, KeyContainer, SymmetricCryptoKey};
use bitwarden_crypto::{AsymmetricCryptoKey, CryptoError, KeyContainer, SymmetricCryptoKey};
#[cfg(feature = "internal")]
use bitwarden_crypto::{AsymmetricEncString, EncString, MasterKey};
use uuid::Uuid;
Expand Down Expand Up @@ -95,22 +95,25 @@ impl EncryptionSettings {
Ok(self)
}

pub fn get_key(&self, org_id: &Option<Uuid>) -> Option<&SymmetricCryptoKey> {
pub fn get_key(&self, org_id: &Option<Uuid>) -> Result<&SymmetricCryptoKey, CryptoError> {
// If we don't have a private key set (to decode multiple org keys), we just use the main
// user key
if self.private_key.is_none() {
return Some(&self.user_key);
return Ok(&self.user_key);
}

match org_id {
Some(org_id) => self.org_keys.get(org_id),
None => Some(&self.user_key),
Some(org_id) => self
.org_keys
.get(org_id)
.ok_or(CryptoError::MissingKey(*org_id)),
None => Ok(&self.user_key),
}
}
}

impl KeyContainer for EncryptionSettings {
fn get_key(&self, org_id: &Option<Uuid>) -> Option<&SymmetricCryptoKey> {
fn get_key(&self, org_id: &Option<Uuid>) -> Result<&SymmetricCryptoKey, CryptoError> {
EncryptionSettings::get_key(self, org_id)
}
}
12 changes: 6 additions & 6 deletions crates/bitwarden-core/src/mobile/crypto.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use serde::{Deserialize, Serialize};
use crate::client::{LoginMethod, UserLoginMethod};
use crate::{
error::{Error, Result},
Client, VaultLocked,
Client,
};

#[cfg(feature = "internal")]
Expand Down Expand Up @@ -185,7 +185,7 @@ pub async fn initialize_org_crypto(client: &Client, req: InitOrgCryptoRequest) -
#[cfg(feature = "internal")]
pub async fn get_user_encryption_key(client: &Client) -> Result<String> {
let enc = client.internal.get_encryption_settings()?;
let user_key = enc.get_key(&None).ok_or(VaultLocked)?;
let user_key = enc.get_key(&None)?;

Ok(user_key.to_base64())
}
Expand All @@ -203,7 +203,7 @@ pub struct UpdatePasswordResponse {

pub fn update_password(client: &Client, new_password: String) -> Result<UpdatePasswordResponse> {
let enc = client.internal.get_encryption_settings()?;
let user_key = enc.get_key(&None).ok_or(VaultLocked)?;
let user_key = enc.get_key(&None)?;

let login_method = client
.internal
Expand Down Expand Up @@ -247,7 +247,7 @@ pub struct DerivePinKeyResponse {
#[cfg(feature = "internal")]
pub fn derive_pin_key(client: &Client, pin: String) -> Result<DerivePinKeyResponse> {
let enc = client.internal.get_encryption_settings()?;
let user_key = enc.get_key(&None).ok_or(VaultLocked)?;
let user_key = enc.get_key(&None)?;

let login_method = client
.internal
Expand All @@ -265,7 +265,7 @@ pub fn derive_pin_key(client: &Client, pin: String) -> Result<DerivePinKeyRespon
#[cfg(feature = "internal")]
pub fn derive_pin_user_key(client: &Client, encrypted_pin: EncString) -> Result<EncString> {
let enc = client.internal.get_encryption_settings()?;
let user_key = enc.get_key(&None).ok_or(VaultLocked)?;
let user_key = enc.get_key(&None)?;

let pin: String = encrypted_pin.decrypt_with_key(user_key)?;
let login_method = client
Expand Down Expand Up @@ -306,7 +306,7 @@ pub(super) fn enroll_admin_password_reset(

let public_key = AsymmetricPublicCryptoKey::from_der(&STANDARD.decode(public_key)?)?;
let enc = client.internal.get_encryption_settings()?;
let key = enc.get_key(&None).ok_or(VaultLocked)?;
let key = enc.get_key(&None)?;

Ok(AsymmetricEncString::encrypt_rsa2048_oaep_sha1(
&key.to_vec(),
Expand Down
5 changes: 3 additions & 2 deletions crates/bitwarden-crypto/src/error.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use std::fmt::Debug;

use thiserror::Error;
use uuid::Uuid;

use crate::fingerprint::FingerprintError;

Expand All @@ -16,8 +17,8 @@ pub enum CryptoError {
InvalidKeyLen,
#[error("The value is not a valid UTF8 String")]
InvalidUtf8String,
#[error("Missing Key")]
MissingKey,
#[error("Missing Key for organization with ID {0}")]
MissingKey(Uuid),

#[error("EncString error, {0}")]
EncString(#[from] EncStringParseError),
Expand Down
8 changes: 4 additions & 4 deletions crates/bitwarden-crypto/src/keys/key_encryptable.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,14 @@ use std::{collections::HashMap, hash::Hash, sync::Arc};
use rayon::prelude::*;
use uuid::Uuid;

use crate::{error::Result, SymmetricCryptoKey};
use crate::{error::Result, CryptoError, SymmetricCryptoKey};

pub trait KeyContainer: Send + Sync {
fn get_key(&self, org_id: &Option<Uuid>) -> Option<&SymmetricCryptoKey>;
fn get_key(&self, org_id: &Option<Uuid>) -> Result<&SymmetricCryptoKey, CryptoError>;
}

impl<T: KeyContainer> KeyContainer for Arc<T> {
fn get_key(&self, org_id: &Option<Uuid>) -> Option<&SymmetricCryptoKey> {
fn get_key(&self, org_id: &Option<Uuid>) -> Result<&SymmetricCryptoKey, CryptoError> {
self.as_ref().get_key(org_id)
}
}
Expand All @@ -20,7 +20,7 @@ pub trait LocateKey {
&self,
enc: &'a dyn KeyContainer,
org_id: &Option<Uuid>,
) -> Option<&'a SymmetricCryptoKey> {
) -> Result<&'a SymmetricCryptoKey, CryptoError> {
enc.get_key(org_id)
}
}
Expand Down
4 changes: 2 additions & 2 deletions crates/bitwarden-exporters/src/export.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use bitwarden_core::{Client, VaultLocked};
use bitwarden_core::Client;
use bitwarden_crypto::KeyDecryptable;
use bitwarden_vault::{Cipher, CipherView, Collection, Folder, FolderView};

Expand All @@ -14,7 +14,7 @@ pub(crate) fn export_vault(
format: ExportFormat,
) -> Result<String, ExportError> {
let enc = client.internal.get_encryption_settings()?;
let key = enc.get_key(&None).ok_or(VaultLocked)?;
let key = enc.get_key(&None)?;

let folders: Vec<FolderView> = folders.decrypt_with_key(key)?;
let folders: Vec<crate::Folder> = folders.into_iter().flat_map(|f| f.try_into()).collect();
Expand Down
4 changes: 2 additions & 2 deletions crates/bitwarden-fido/src/authenticator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -483,7 +483,7 @@ impl passkey::authenticator::CredentialStore for CredentialStoreImpl<'_> {
.replace(selected.clone());

// Encrypt the updated cipher before sending it to the clients to be stored
let key = enc.get_key(&selected.organization_id).ok_or(VaultLocked)?;
let key = enc.get_key(&selected.organization_id)?;
let encrypted = selected.encrypt_with_key(key)?;

this.authenticator
Expand Down Expand Up @@ -557,7 +557,7 @@ impl passkey::authenticator::CredentialStore for CredentialStoreImpl<'_> {
.replace(selected.clone());

// Encrypt the updated cipher before sending it to the clients to be stored
let key = enc.get_key(&selected.organization_id).ok_or(VaultLocked)?;
let key = enc.get_key(&selected.organization_id)?;
let encrypted = selected.encrypt_with_key(key)?;

this.authenticator
Expand Down
12 changes: 6 additions & 6 deletions crates/bitwarden-send/src/client_sends.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use std::path::Path;

use bitwarden_core::{Client, Error, VaultLocked};
use bitwarden_core::{Client, Error};
use bitwarden_crypto::{EncString, KeyDecryptable, KeyEncryptable};

use crate::{Send, SendListView, SendView};
Expand All @@ -16,7 +16,7 @@ impl<'a> ClientSends<'a> {

pub fn decrypt(&self, send: Send) -> Result<SendView, Error> {
let enc = self.client.internal.get_encryption_settings()?;
let key = enc.get_key(&None).ok_or(VaultLocked)?;
let key = enc.get_key(&None)?;

let send_view = send.decrypt_with_key(key)?;

Expand All @@ -25,7 +25,7 @@ impl<'a> ClientSends<'a> {

pub fn decrypt_list(&self, sends: Vec<Send>) -> Result<Vec<SendListView>, Error> {
let enc = self.client.internal.get_encryption_settings()?;
let key = enc.get_key(&None).ok_or(VaultLocked)?;
let key = enc.get_key(&None)?;

let send_views = sends.decrypt_with_key(key)?;

Expand All @@ -46,7 +46,7 @@ impl<'a> ClientSends<'a> {

pub fn decrypt_buffer(&self, send: Send, encrypted_buffer: &[u8]) -> Result<Vec<u8>, Error> {
let enc = self.client.internal.get_encryption_settings()?;
let key = enc.get_key(&None).ok_or(VaultLocked)?;
let key = enc.get_key(&None)?;
let key = Send::get_key(&send.key, key)?;

let buf = EncString::from_buffer(encrypted_buffer)?;
Expand All @@ -55,7 +55,7 @@ impl<'a> ClientSends<'a> {

pub fn encrypt(&self, send_view: SendView) -> Result<Send, Error> {
let enc = self.client.internal.get_encryption_settings()?;
let key = enc.get_key(&None).ok_or(VaultLocked)?;
let key = enc.get_key(&None)?;

let send = send_view.encrypt_with_key(key)?;

Expand All @@ -76,7 +76,7 @@ impl<'a> ClientSends<'a> {

pub fn encrypt_buffer(&self, send: Send, buffer: &[u8]) -> Result<Vec<u8>, Error> {
let enc = self.client.internal.get_encryption_settings()?;
let key = enc.get_key(&None).ok_or(VaultLocked)?;
let key = enc.get_key(&None)?;
let key = Send::get_key(&send.key, key)?;

let encrypted = buffer.encrypt_with_key(&key)?;
Expand Down
9 changes: 7 additions & 2 deletions crates/bitwarden-send/src/send.rs
Original file line number Diff line number Diff line change
Expand Up @@ -375,8 +375,13 @@ mod tests {
}
}
impl KeyContainer for MockKeyContainer {
fn get_key<'a>(&'a self, org_id: &Option<Uuid>) -> Option<&'a SymmetricCryptoKey> {
self.0.get(org_id)
fn get_key<'a>(
&'a self,
org_id: &Option<Uuid>,
) -> Result<&'a SymmetricCryptoKey, CryptoError> {
self.0
.get(org_id)
.ok_or(CryptoError::MissingKey(org_id.unwrap_or_default()))
}
}

Expand Down
6 changes: 2 additions & 4 deletions crates/bitwarden-sm/src/projects/create.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use bitwarden_api_api::models::ProjectCreateRequestModel;
use bitwarden_core::{validate_only_whitespaces, Client, Error, VaultLocked};
use bitwarden_core::{validate_only_whitespaces, Client, Error};
use bitwarden_crypto::KeyEncryptable;
use schemars::JsonSchema;
use serde::{Deserialize, Serialize};
Expand All @@ -24,9 +24,7 @@ pub(crate) async fn create_project(
input.validate()?;

let enc = client.internal.get_encryption_settings()?;
let key = enc
.get_key(&Some(input.organization_id))
.ok_or(VaultLocked)?;
let key = enc.get_key(&Some(input.organization_id))?;

let project = Some(ProjectCreateRequestModel {
name: input.name.clone().trim().encrypt_with_key(key)?.to_string(),
Expand Down
6 changes: 2 additions & 4 deletions crates/bitwarden-sm/src/projects/project_response.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use bitwarden_api_api::models::ProjectResponseModel;
use bitwarden_core::{client::encryption_settings::EncryptionSettings, require, Error};
use bitwarden_crypto::{CryptoError, EncString, KeyDecryptable};
use bitwarden_crypto::{EncString, KeyDecryptable};
use chrono::{DateTime, Utc};
use schemars::JsonSchema;
use serde::{Deserialize, Serialize};
Expand All @@ -22,9 +22,7 @@ impl ProjectResponse {
enc: &EncryptionSettings,
) -> Result<Self, Error> {
let organization_id = require!(response.organization_id);
let enc_key = enc
.get_key(&Some(organization_id))
.ok_or(CryptoError::MissingKey)?;
let enc_key = enc.get_key(&Some(organization_id))?;

let name = require!(response.name)
.parse::<EncString>()?
Expand Down
6 changes: 2 additions & 4 deletions crates/bitwarden-sm/src/projects/update.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use bitwarden_api_api::models::ProjectUpdateRequestModel;
use bitwarden_core::{validate_only_whitespaces, Client, Error, VaultLocked};
use bitwarden_core::{validate_only_whitespaces, Client, Error};
use bitwarden_crypto::KeyEncryptable;
use schemars::JsonSchema;
use serde::{Deserialize, Serialize};
Expand All @@ -26,9 +26,7 @@ pub(crate) async fn update_project(
input.validate()?;

let enc = client.internal.get_encryption_settings()?;
let key = enc
.get_key(&Some(input.organization_id))
.ok_or(VaultLocked)?;
let key = enc.get_key(&Some(input.organization_id))?;

let project = Some(ProjectUpdateRequestModel {
name: input.name.clone().trim().encrypt_with_key(key)?.to_string(),
Expand Down
6 changes: 2 additions & 4 deletions crates/bitwarden-sm/src/secrets/create.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use bitwarden_api_api::models::SecretCreateRequestModel;
use bitwarden_core::{validate_only_whitespaces, Client, Error, VaultLocked};
use bitwarden_core::{validate_only_whitespaces, Client, Error};
use bitwarden_crypto::KeyEncryptable;
use schemars::JsonSchema;
use serde::{Deserialize, Serialize};
Expand Down Expand Up @@ -32,9 +32,7 @@ pub(crate) async fn create_secret(
input.validate()?;

let enc = client.internal.get_encryption_settings()?;
let key = enc
.get_key(&Some(input.organization_id))
.ok_or(VaultLocked)?;
let key = enc.get_key(&Some(input.organization_id))?;

let secret = Some(SecretCreateRequestModel {
key: input.key.clone().trim().encrypt_with_key(key)?.to_string(),
Expand Down
6 changes: 2 additions & 4 deletions crates/bitwarden-sm/src/secrets/list.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use bitwarden_core::{
client::{encryption_settings::EncryptionSettings, Client},
require, Error,
};
use bitwarden_crypto::{CryptoError, EncString, KeyDecryptable};
use bitwarden_crypto::{EncString, KeyDecryptable};
use schemars::JsonSchema;
use serde::{Deserialize, Serialize};
use uuid::Uuid;
Expand Down Expand Up @@ -93,9 +93,7 @@ impl SecretIdentifierResponse {
enc: &EncryptionSettings,
) -> Result<SecretIdentifierResponse, Error> {
let organization_id = require!(response.organization_id);
let enc_key = enc
.get_key(&Some(organization_id))
.ok_or(CryptoError::MissingKey)?;
let enc_key = enc.get_key(&Some(organization_id))?;

let key = require!(response.key)
.parse::<EncString>()?
Expand Down
Loading

0 comments on commit 69c1735

Please sign in to comment.