Skip to content
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

[PM-6760] Add support for URI checksums #662

Merged
merged 4 commits into from
Mar 21, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 29 additions & 3 deletions crates/bitwarden/src/vault/cipher/cipher.rs
Original file line number Diff line number Diff line change
Expand Up @@ -139,10 +139,15 @@
}

impl KeyEncryptable<SymmetricCryptoKey, Cipher> for CipherView {
fn encrypt_with_key(self, key: &SymmetricCryptoKey) -> Result<Cipher, CryptoError> {
fn encrypt_with_key(mut self, key: &SymmetricCryptoKey) -> Result<Cipher, CryptoError> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to actually mutate self? We should be able to mutate just the URIs in login.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could change the mutability to only affect the login like this, but to me it seems harder to read:

This code will be nicer when moved to LoginUriView, but we can't do that until we decide to unconditionally generate the checksums, as the LoginUriView doesn't have knowledge of whether a cipher key is used or not.

    fn encrypt_with_key(self, key: &SymmetricCryptoKey) -> Result<Cipher, CryptoError> {
        let ciphers_key = Cipher::get_cipher_key(key, &self.key)?;
        let key = ciphers_key.as_ref().unwrap_or(key);

        let mut login = self.login;

        // For compatibility reasons, we only create checksums for ciphers that have a key
        if ciphers_key.is_some() {
            login.generate_checksums();
        }

        Ok(Cipher {
            id: self.id,
            organization_id: self.organization_id,
            ...
            login: login.encrypt_with_key(key)?,

let ciphers_key = Cipher::get_cipher_key(key, &self.key)?;
let key = ciphers_key.as_ref().unwrap_or(key);

// For compatibility reasons, we only create checksums for ciphers that have a key
if ciphers_key.is_some() {
self.generate_checksums();
}

Ok(Cipher {
id: self.id,
organization_id: self.organization_id,
Expand Down Expand Up @@ -177,7 +182,7 @@
let ciphers_key = Cipher::get_cipher_key(key, &self.key)?;
let key = ciphers_key.as_ref().unwrap_or(key);

Ok(CipherView {
let mut cipher = CipherView {
id: self.id,
organization_id: self.organization_id,
folder_id: self.folder_id,
Expand All @@ -202,7 +207,14 @@
creation_date: self.creation_date,
deleted_date: self.deleted_date,
revision_date: self.revision_date,
})
};

// For compatibility we only remove URLs with invalid checksums if the cipher has a key
if ciphers_key.is_some() {
cipher.remove_invalid_checksums();
}

Ok(cipher)
}
}

Expand Down Expand Up @@ -299,6 +311,20 @@
self.key = Some(new_key.to_vec().encrypt_with_key(key)?);
Ok(())
}

pub fn generate_checksums(&mut self) {
if let Some(uris) = self.login.as_mut().and_then(|l| l.uris.as_mut()) {
for uri in uris {
uri.generate_checksum();
}

Check warning on line 319 in crates/bitwarden/src/vault/cipher/cipher.rs

View check run for this annotation

Codecov / codecov/patch

crates/bitwarden/src/vault/cipher/cipher.rs#L317-L319

Added lines #L317 - L319 were not covered by tests
}
}

pub fn remove_invalid_checksums(&mut self) {
if let Some(uris) = self.login.as_mut().and_then(|l| l.uris.as_mut()) {
uris.retain(|u| u.is_checksum_valid());

Check warning on line 325 in crates/bitwarden/src/vault/cipher/cipher.rs

View check run for this annotation

Codecov / codecov/patch

crates/bitwarden/src/vault/cipher/cipher.rs#L325

Added line #L325 was not covered by tests
}
}
}

impl KeyDecryptable<SymmetricCryptoKey, CipherListView> for Cipher {
Expand Down
83 changes: 83 additions & 0 deletions crates/bitwarden/src/vault/cipher/login.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use base64::{engine::general_purpose::STANDARD, Engine};
use bitwarden_api_api::models::{CipherLoginModel, CipherLoginUriModel};
use bitwarden_crypto::{
CryptoError, EncString, KeyDecryptable, KeyEncryptable, SymmetricCryptoKey,
Expand Down Expand Up @@ -28,6 +29,7 @@
pub struct LoginUri {
pub uri: Option<EncString>,
pub r#match: Option<UriMatchType>,
pub uri_checksum: Option<EncString>,
}

#[derive(Serialize, Deserialize, Debug, JsonSchema)]
Expand All @@ -36,6 +38,35 @@
pub struct LoginUriView {
pub uri: Option<String>,
pub r#match: Option<UriMatchType>,
pub uri_checksum: Option<String>,
}

impl LoginUriView {
pub(crate) fn is_checksum_valid(&self) -> bool {
let Some(uri) = &self.uri else {
return false;

Check warning on line 47 in crates/bitwarden/src/vault/cipher/login.rs

View check run for this annotation

Codecov / codecov/patch

crates/bitwarden/src/vault/cipher/login.rs#L47

Added line #L47 was not covered by tests
};
let Some(cs) = &self.uri_checksum else {
return false;
};
let Ok(cs) = STANDARD.decode(cs) else {
return false;

Check warning on line 53 in crates/bitwarden/src/vault/cipher/login.rs

View check run for this annotation

Codecov / codecov/patch

crates/bitwarden/src/vault/cipher/login.rs#L53

Added line #L53 was not covered by tests
};

use sha2::Digest;
let uri_hash = sha2::Sha256::new().chain_update(uri.as_bytes()).finalize();

uri_hash.as_slice() == cs
}

pub(crate) fn generate_checksum(&mut self) {
if let Some(uri) = &self.uri {
use sha2::Digest;
let uri_hash = sha2::Sha256::new().chain_update(uri.as_bytes()).finalize();
let uri_hash = STANDARD.encode(uri_hash.as_slice());
self.uri_checksum = Some(uri_hash);
}
}
}

#[derive(Serialize, Deserialize, Debug, JsonSchema, Clone)]
Expand Down Expand Up @@ -93,6 +124,7 @@
Ok(LoginUri {
uri: self.uri.encrypt_with_key(key)?,
r#match: self.r#match,
uri_checksum: self.uri_checksum.encrypt_with_key(key)?,

Check warning on line 127 in crates/bitwarden/src/vault/cipher/login.rs

View check run for this annotation

Codecov / codecov/patch

crates/bitwarden/src/vault/cipher/login.rs#L127

Added line #L127 was not covered by tests
})
}
}
Expand All @@ -116,6 +148,7 @@
Ok(LoginUriView {
uri: self.uri.decrypt_with_key(key)?,
r#match: self.r#match,
uri_checksum: self.uri_checksum.decrypt_with_key(key)?,

Check warning on line 151 in crates/bitwarden/src/vault/cipher/login.rs

View check run for this annotation

Codecov / codecov/patch

crates/bitwarden/src/vault/cipher/login.rs#L151

Added line #L151 was not covered by tests
})
}
}
Expand Down Expand Up @@ -166,6 +199,7 @@
Ok(Self {
uri: EncString::try_from_optional(uri.uri)?,
r#match: uri.r#match.map(|m| m.into()),
uri_checksum: EncString::try_from_optional(uri.uri_checksum)?,

Check warning on line 202 in crates/bitwarden/src/vault/cipher/login.rs

View check run for this annotation

Codecov / codecov/patch

crates/bitwarden/src/vault/cipher/login.rs#L202

Added line #L202 was not covered by tests
})
}
}
Expand Down Expand Up @@ -208,3 +242,52 @@
})
}
}

#[cfg(test)]
mod tests {
#[test]
fn test_valid_checksum() {
let uri = super::LoginUriView {
uri: Some("https://example.com".to_string()),
r#match: Some(super::UriMatchType::Domain),
uri_checksum: Some("EAaArVRs5qV39C9S3zO0z9ynVoWeZkuNfeMpsVDQnOk=".to_string()),
};
assert!(uri.is_checksum_valid());
}

#[test]
fn test_invalid_checksum() {
let uri = super::LoginUriView {
uri: Some("https://example.com".to_string()),
r#match: Some(super::UriMatchType::Domain),
uri_checksum: Some("UtSgIv8LYfEdOu7yqjF7qXWhmouYGYC8RSr7/ryZg5Q=".to_string()),
};
assert!(!uri.is_checksum_valid());
}

#[test]
fn test_missing_checksum() {
let uri = super::LoginUriView {
uri: Some("https://example.com".to_string()),
r#match: Some(super::UriMatchType::Domain),
uri_checksum: None,
};
assert!(!uri.is_checksum_valid());
}

#[test]
fn test_generate_checksum() {
let mut uri = super::LoginUriView {
uri: Some("https://test.com".to_string()),
r#match: Some(super::UriMatchType::Domain),
uri_checksum: None,
};

uri.generate_checksum();

assert_eq!(
uri.uri_checksum.unwrap().as_str(),
"OWk2vQvwYD1nhLZdA+ltrpBWbDa2JmHyjUEWxRZSS8w="
);
}
}
Loading