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-14252] Switch to oo7 and drop libsecret #11900

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
312 changes: 128 additions & 184 deletions apps/desktop/desktop_native/Cargo.lock

Large diffs are not rendered by default.

6 changes: 2 additions & 4 deletions apps/desktop/desktop_native/core/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,6 @@ sys = [
"dep:core-foundation",
"dep:security-framework",
"dep:security-framework-sys",
"dep:gio",
"dep:libsecret",
"dep:zbus",
"dep:zbus_polkit",
]
Expand Down Expand Up @@ -84,7 +82,7 @@ security-framework = { version = "=3.0.0", optional = true }
security-framework-sys = { version = "=2.12.0", optional = true }

[target.'cfg(target_os = "linux")'.dependencies]
gio = { version = "=0.19.5", optional = true }
libsecret = { version = "=0.5.0", optional = true }
Copy link
Member

Choose a reason for hiding this comment

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

If we're removing the libsecret crate can we also remove the dependency to the libsecret system package?

"depends": ["libnotify4", "libxtst6", "libnss3", "libsecret-1-0", "libxss1"]

sudo apt-get -y install pkg-config libxss-dev libsecret-1-dev rpm musl-dev musl-tools

sudo apt-get -y install pkg-config libxss-dev libsecret-1-dev rpm

oo7 = "0.3.3"
Copy link
Member

Choose a reason for hiding this comment

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

We should pin the dep like the rest of them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pinned the dependency now, but i'm not getting any downgrade warnings/conflicts? Not sure if it matters but i'm on cargo/rustc 1.82.0

Copy link
Member

Choose a reason for hiding this comment

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

Hmm I think I was testing in a temporary branch where I merged your argon2 PR and this, so maybe there was something weird during the merge, we can ignore it if you haven't had any issues.


zbus = { version = "=4.4.0", optional = true }
zbus_polkit = { version = "=4.0.0", optional = true }
4 changes: 2 additions & 2 deletions apps/desktop/desktop_native/core/src/biometric/macos.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,15 @@ impl super::BiometricTrait for Biometric {
bail!("platform not supported");
}

fn get_biometric_secret(
async fn get_biometric_secret(
_service: &str,
_account: &str,
_key_material: Option<KeyMaterial>,
) -> Result<String> {
bail!("platform not supported");
}

fn set_biometric_secret(
async fn set_biometric_secret(
_service: &str,
_account: &str,
_secret: &str,
Expand Down
6 changes: 4 additions & 2 deletions apps/desktop/desktop_native/core/src/biometric/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,14 +28,16 @@ pub trait BiometricTrait {
#[allow(async_fn_in_trait)]
async fn available() -> Result<bool>;
fn derive_key_material(secret: Option<&str>) -> Result<OsDerivedKey>;
fn set_biometric_secret(
#[allow(async_fn_in_trait)]
Copy link
Member

Choose a reason for hiding this comment

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

Can we lift the #[allow()] to the trait to avoid repeating it multiple times?

async fn set_biometric_secret(
service: &str,
account: &str,
secret: &str,
key_material: Option<KeyMaterial>,
iv_b64: &str,
) -> Result<String>;
fn get_biometric_secret(
#[allow(async_fn_in_trait)]
async fn get_biometric_secret(
service: &str,
account: &str,
key_material: Option<KeyMaterial>,
Expand Down
8 changes: 4 additions & 4 deletions apps/desktop/desktop_native/core/src/biometric/unix.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ impl super::BiometricTrait for Biometric {
Ok(OsDerivedKey { key_b64, iv_b64 })
}

fn set_biometric_secret(
async fn set_biometric_secret(
service: &str,
account: &str,
secret: &str,
Expand All @@ -85,11 +85,11 @@ impl super::BiometricTrait for Biometric {
))?;

let encrypted_secret = encrypt(secret, &key_material, iv_b64)?;
crate::password::set_password(service, account, &encrypted_secret)?;
crate::password::set_password(service, account, &encrypted_secret).await?;
Ok(encrypted_secret)
}

fn get_biometric_secret(
async fn get_biometric_secret(
service: &str,
account: &str,
key_material: Option<KeyMaterial>,
Expand All @@ -98,7 +98,7 @@ impl super::BiometricTrait for Biometric {
"Key material is required for polkit protected keys"
))?;

let encrypted_secret = crate::password::get_password(service, account)?;
let encrypted_secret = crate::password::get_password(service, account).await?;
let secret = CipherString::from_str(&encrypted_secret)?;
return Ok(decrypt(&secret, &key_material)?);
}
Expand Down
42 changes: 20 additions & 22 deletions apps/desktop/desktop_native/core/src/biometric/windows.rs
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ impl super::BiometricTrait for Biometric {
Ok(OsDerivedKey { key_b64, iv_b64 })
}

fn set_biometric_secret(
async fn set_biometric_secret(
service: &str,
account: &str,
secret: &str,
Expand All @@ -131,11 +131,11 @@ impl super::BiometricTrait for Biometric {
))?;

let encrypted_secret = encrypt(secret, &key_material, iv_b64)?;
crate::password::set_password(service, account, &encrypted_secret)?;
crate::password::set_password(service, account, &encrypted_secret).await?;
Ok(encrypted_secret)
}

fn get_biometric_secret(
async fn get_biometric_secret(
service: &str,
account: &str,
key_material: Option<KeyMaterial>,
Expand All @@ -144,7 +144,7 @@ impl super::BiometricTrait for Biometric {
"Key material is required for Windows Hello protected keys"
))?;

let encrypted_secret = crate::password::get_password(service, account)?;
let encrypted_secret = crate::password::get_password(service, account).await?;
match CipherString::from_str(&encrypted_secret) {
Ok(secret) => {
// If the secret is a CipherString, it is encrypted and we need to decrypt it.
Expand Down Expand Up @@ -290,57 +290,55 @@ mod tests {
assert_eq!(decrypt(&secret, &key_material).unwrap(), "secret")
}

#[test]
fn get_biometric_secret_requires_key() {
let result = <Biometric as BiometricTrait>::get_biometric_secret("", "", None);
#[tokio::test]
async fn get_biometric_secret_requires_key() {
let result = <Biometric as BiometricTrait>::get_biometric_secret("", "", None).await;
assert!(result.is_err());
assert_eq!(
result.unwrap_err().to_string(),
"Key material is required for Windows Hello protected keys"
);
}

#[test]
fn get_biometric_secret_handles_unencrypted_secret() {
scopeguard::defer! {
crate::password::delete_password("test", "test").unwrap();
}
#[tokio::test]
async fn get_biometric_secret_handles_unencrypted_secret() {
let test = "test";
let secret = "password";
let key_material = KeyMaterial {
os_key_part_b64: "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=".to_owned(),
client_key_part_b64: Some("AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=".to_owned()),
};
crate::password::set_password(test, test, secret).unwrap();
crate::password::set_password(test, test, secret).await.unwrap();
let result =
<Biometric as BiometricTrait>::get_biometric_secret(test, test, Some(key_material))
.await
.unwrap();
crate::password::delete_password("test", "test").await.unwrap();
assert_eq!(result, secret);
}

#[test]
fn get_biometric_secret_handles_encrypted_secret() {
scopeguard::defer! {
crate::password::delete_password("test", "test").unwrap();
}
#[tokio::test]
async fn get_biometric_secret_handles_encrypted_secret() {
let test = "test";
let secret =
CipherString::from_str("0.l9fhDUP/wDJcKwmEzcb/3w==|uP4LcqoCCj5FxBDP77NV6Q==").unwrap(); // output from test_encrypt
let key_material = KeyMaterial {
os_key_part_b64: "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=".to_owned(),
client_key_part_b64: Some("AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=".to_owned()),
};
crate::password::set_password(test, test, &secret.to_string()).unwrap();
crate::password::set_password(test, test, &secret.to_string()).await.unwrap();

let result =
<Biometric as BiometricTrait>::get_biometric_secret(test, test, Some(key_material))
.await
.unwrap();
crate::password::delete_password("test", "test").await.unwrap();
assert_eq!(result, "secret");
}

#[test]
fn set_biometric_secret_requires_key() {
let result = <Biometric as BiometricTrait>::set_biometric_secret("", "", "", None, "");
#[tokio::test]
async fn set_biometric_secret_requires_key() {
let result = <Biometric as BiometricTrait>::set_biometric_secret("", "", "", None, "").await;
assert!(result.is_err());
assert_eq!(
result.unwrap_err().to_string(),
Expand Down
31 changes: 13 additions & 18 deletions apps/desktop/desktop_native/core/src/password/macos.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,45 +3,40 @@ use security_framework::passwords::{
delete_generic_password, get_generic_password, set_generic_password,
};

pub fn get_password(service: &str, account: &str) -> Result<String> {
pub async fn get_password(service: &str, account: &str) -> Result<String> {
let result = String::from_utf8(get_generic_password(&service, &account)?)?;
Ok(result)
}

pub fn get_password_keytar(service: &str, account: &str) -> Result<String> {
get_password(service, account)
}

pub fn set_password(service: &str, account: &str, password: &str) -> Result<()> {
pub async fn set_password(service: &str, account: &str, password: &str) -> Result<()> {
let result = set_generic_password(&service, &account, password.as_bytes())?;
Ok(result)
}

pub fn delete_password(service: &str, account: &str) -> Result<()> {
pub async fn delete_password(service: &str, account: &str) -> Result<()> {
let result = delete_generic_password(&service, &account)?;
Ok(result)
}

pub fn is_available() -> Result<bool> {
pub async fn is_available() -> Result<bool> {
Ok(true)
}

#[cfg(test)]
mod tests {
use super::*;

#[test]
fn test() {
scopeguard::defer!(delete_password("BitwardenTest", "BitwardenTest").unwrap_or({}););
set_password("BitwardenTest", "BitwardenTest", "Random").unwrap();
#[tokio::test]
async fn test() {
set_password("BitwardenTest", "BitwardenTest", "Random").await.unwrap();
assert_eq!(
"Random",
get_password("BitwardenTest", "BitwardenTest").unwrap()
get_password("BitwardenTest", "BitwardenTest").await.unwrap()
);
delete_password("BitwardenTest", "BitwardenTest").unwrap();
delete_password("BitwardenTest", "BitwardenTest").await.unwrap();

// Ensure password is deleted
match get_password("BitwardenTest", "BitwardenTest") {
match get_password("BitwardenTest", "BitwardenTest").await {
Ok(_) => panic!("Got a result"),
Err(e) => assert_eq!(
"The specified item could not be found in the keychain.",
Expand All @@ -50,9 +45,9 @@ mod tests {
}
}

#[test]
fn test_error_no_password() {
match get_password("Unknown", "Unknown") {
#[tokio::test]
async fn test_error_no_password() {
match get_password("Unknown", "Unknown").await {
Ok(_) => panic!("Got a result"),
Err(e) => assert_eq!(
"The specified item could not be found in the keychain.",
Expand Down
Loading
Loading