From c98b86ebbaefb5982de82e6e991071d321800faa Mon Sep 17 00:00:00 2001 From: Tolga Ergin Date: Tue, 15 Sep 2026 21:13:07 +0100 Subject: [PATCH 1/2] Scope macOS auth to the shared Data Protection Keychain --- Cargo.lock | 1 + crates/lpm-auth/Cargo.toml | 3 +- crates/lpm-auth/src/credential_authority.rs | 58 ++++- crates/lpm-auth/src/keychain_migration.rs | 240 ++++++++++++++++++++ crates/lpm-auth/src/lib.rs | 154 ++++++------- crates/lpm-auth/src/macos_keychain.rs | 217 ++++++++++++++++++ 6 files changed, 590 insertions(+), 83 deletions(-) create mode 100644 crates/lpm-auth/src/keychain_migration.rs create mode 100644 crates/lpm-auth/src/macos_keychain.rs diff --git a/Cargo.lock b/Cargo.lock index ca3c3b91..6cff0d25 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3324,6 +3324,7 @@ dependencies = [ "scrypt", "secrecy", "security-framework 3.7.0", + "security-framework-sys", "serde", "serde_json", "sha2", diff --git a/crates/lpm-auth/Cargo.toml b/crates/lpm-auth/Cargo.toml index d21f418d..d0d903b9 100644 --- a/crates/lpm-auth/Cargo.toml +++ b/crates/lpm-auth/Cargo.toml @@ -52,7 +52,8 @@ keyring = { workspace = true, features = ["vendored"] } keyring = { workspace = true, features = ["apple-native"] } core-foundation = "0.10" objc2-local-authentication = { version = "0.3", default-features = false, features = ["std", "LAContext"] } -security-framework = "3" +security-framework = { version = "3", features = ["OSX_10_15"] } +security-framework-sys = { version = "2", features = ["OSX_10_15"] } [target.'cfg(windows)'.dependencies] keyring = { workspace = true, features = ["windows-native"] } diff --git a/crates/lpm-auth/src/credential_authority.rs b/crates/lpm-auth/src/credential_authority.rs index 07c65ea0..515f949e 100644 --- a/crates/lpm-auth/src/credential_authority.rs +++ b/crates/lpm-auth/src/credential_authority.rs @@ -31,9 +31,20 @@ impl CredentialKind { #[serde(rename_all = "snake_case")] pub(super) enum CredentialBackend { Keychain, + SharedKeychain, EncryptedFileFallback, } +impl CredentialBackend { + pub(super) const fn current_keychain() -> Self { + if cfg!(target_os = "macos") { + Self::SharedKeychain + } else { + Self::Keychain + } + } +} + #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[serde(tag = "state", rename_all = "snake_case")] pub(super) enum CredentialAuthority { @@ -41,6 +52,8 @@ pub(super) enum CredentialAuthority { backend: CredentialBackend, credential_digest: String, stale_file_cleanup_pending: bool, + #[serde(default)] + legacy_keychain_cleanup_pending: bool, }, Revoked, } @@ -51,15 +64,56 @@ impl CredentialAuthority { backend, credential_digest: token_digest(token), stale_file_cleanup_pending: false, + legacy_keychain_cleanup_pending: false, } } pub(super) fn keychain_cleanup_pending(token: &str) -> Self { Self::Active { - backend: CredentialBackend::Keychain, + backend: CredentialBackend::current_keychain(), credential_digest: token_digest(token), stale_file_cleanup_pending: true, + legacy_keychain_cleanup_pending: cfg!(target_os = "macos"), + } + } + + pub(super) fn committed_keychain(token: &str) -> Self { + let mut value = Self::active(CredentialBackend::current_keychain(), token); + if let Self::Active { + legacy_keychain_cleanup_pending, + .. + } = &mut value + { + *legacy_keychain_cleanup_pending = cfg!(target_os = "macos"); } + value + } + + #[cfg(any(target_os = "macos", test))] + pub(super) fn with_shared_keychain_cleanup(&self, pending: bool) -> Self { + let mut next = self.clone(); + if let Self::Active { + backend, + legacy_keychain_cleanup_pending, + .. + } = &mut next + { + *backend = CredentialBackend::SharedKeychain; + *legacy_keychain_cleanup_pending = pending; + } + next + } + + #[cfg(any(target_os = "macos", test))] + pub(super) fn has_pending_legacy_keychain_cleanup(&self) -> bool { + matches!( + self, + Self::Active { + backend: CredentialBackend::SharedKeychain, + legacy_keychain_cleanup_pending: true, + .. + } + ) } pub(super) fn backend(&self) -> Option { @@ -82,7 +136,7 @@ impl CredentialAuthority { matches!( self, Self::Active { - backend: CredentialBackend::Keychain, + backend: CredentialBackend::Keychain | CredentialBackend::SharedKeychain, stale_file_cleanup_pending: true, .. } diff --git a/crates/lpm-auth/src/keychain_migration.rs b/crates/lpm-auth/src/keychain_migration.rs new file mode 100644 index 00000000..1740c5d2 --- /dev/null +++ b/crates/lpm-auth/src/keychain_migration.rs @@ -0,0 +1,240 @@ +use crate::credential_authority::{CredentialAuthority, CredentialBackend}; + +trait MigrationBackend { + fn read_legacy(&mut self) -> Result, String>; + fn write_shared(&mut self, token: &str) -> Result<(), String>; + fn read_shared(&mut self) -> Result, String>; + fn save_authority(&mut self, authority: &CredentialAuthority) -> Result<(), String>; + fn delete_legacy(&mut self) -> Result<(), String>; +} + +fn migrate( + authority: &mut CredentialAuthority, + backend: &mut impl MigrationBackend, +) -> Result<(), String> { + let mut verified = false; + if authority.backend() == Some(CredentialBackend::Keychain) { + let token = backend + .read_legacy()? + .ok_or("legacy Keychain credential is unavailable")?; + if !authority.matches_token(&token) { + return Err( + "legacy Keychain credential does not match its authority record".to_owned(), + ); + } + backend.write_shared(&token)?; + if backend.read_shared()?.as_deref() != Some(token.as_str()) { + return Err("shared Keychain migration verification failed".to_owned()); + } + let next = authority.with_shared_keychain_cleanup(true); + backend.save_authority(&next)?; + *authority = next; + verified = true; + } + if authority.has_pending_legacy_keychain_cleanup() { + if !verified { + let token = backend + .read_shared()? + .ok_or("shared Keychain credential is unavailable")?; + if !authority.matches_token(&token) { + return Err( + "shared Keychain credential does not match its authority record".to_owned(), + ); + } + } + backend.delete_legacy()?; + let next = authority.with_shared_keychain_cleanup(false); + backend.save_authority(&next)?; + *authority = next; + } + Ok(()) +} + +#[cfg(target_os = "macos")] +pub(super) fn prepare( + registry: &str, + kind: crate::credential_authority::CredentialKind, + account: &str, + authority: &mut CredentialAuthority, + notice: impl FnMut(), +) -> Result<(), String> { + struct Native<'a, F> { + registry: &'a str, + kind: crate::credential_authority::CredentialKind, + account: &'a str, + service: std::borrow::Cow<'static, str>, + notice: F, + } + impl MigrationBackend for Native<'_, F> { + fn read_legacy(&mut self) -> Result, String> { + use crate::macos_keychain::{self, Scope}; + match macos_keychain::read(&self.service, self.account, Scope::Legacy, false) { + Err(error) if error.code() == crate::ERR_SEC_INTERACTION_NOT_ALLOWED => { + (self.notice)(); + macos_keychain::read(&self.service, self.account, Scope::Legacy, true) + .map_err(|error| format!("legacy Keychain read failed: {error}")) + } + result => result.map_err(|error| format!("legacy Keychain read failed: {error}")), + } + } + fn write_shared(&mut self, token: &str) -> Result<(), String> { + crate::macos_keychain::write(&self.service, self.account, token) + .map_err(|error| format!("shared Keychain migration failed: {error}")) + } + fn read_shared(&mut self) -> Result, String> { + crate::macos_keychain::read( + &self.service, + self.account, + crate::macos_keychain::Scope::Shared, + false, + ) + .map_err(|error| format!("shared Keychain read failed: {error}")) + } + fn save_authority(&mut self, authority: &CredentialAuthority) -> Result<(), String> { + crate::credential_authority::set(self.registry, self.kind, authority.clone()) + } + fn delete_legacy(&mut self) -> Result<(), String> { + crate::macos_keychain::delete( + &self.service, + self.account, + crate::macos_keychain::Scope::Legacy, + ) + .map_err(|error| format!("legacy Keychain cleanup failed: {error}")) + } + } + migrate( + authority, + &mut Native { + registry, + kind, + account, + service: crate::keychain_service(), + notice, + }, + ) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[derive(Default)] + struct Memory { + legacy: Option, + shared: Option, + saved: Option, + fail_write: bool, + fail_delete: bool, + fail_save: bool, + corrupt_copy: bool, + legacy_reads: usize, + writes: usize, + } + impl MigrationBackend for Memory { + fn read_legacy(&mut self) -> Result, String> { + self.legacy_reads += 1; + Ok(self.legacy.clone()) + } + fn write_shared(&mut self, token: &str) -> Result<(), String> { + self.writes += 1; + if self.fail_write { + return Err("write failed".into()); + } + self.shared = Some(if self.corrupt_copy { "corrupt" } else { token }.to_owned()); + Ok(()) + } + fn read_shared(&mut self) -> Result, String> { + Ok(self.shared.clone()) + } + fn save_authority(&mut self, authority: &CredentialAuthority) -> Result<(), String> { + if self.fail_save { + return Err("save failed".into()); + } + self.saved = Some(authority.clone()); + Ok(()) + } + fn delete_legacy(&mut self) -> Result<(), String> { + if self.fail_delete { + return Err("delete failed".into()); + } + self.legacy = None; + Ok(()) + } + } + fn legacy() -> (CredentialAuthority, Memory) { + ( + CredentialAuthority::active(CredentialBackend::Keychain, "valid"), + Memory { + legacy: Some("valid".into()), + ..Memory::default() + }, + ) + } + + #[test] + fn matching_legacy_credential_moves_to_shared_storage() { + let (mut authority, mut memory) = legacy(); + migrate(&mut authority, &mut memory).unwrap(); + assert_eq!(authority.backend(), Some(CredentialBackend::SharedKeychain)); + assert!(!authority.has_pending_legacy_keychain_cleanup()); + assert_eq!(memory.shared.as_deref(), Some("valid")); + assert!(memory.legacy.is_none()); + assert_eq!(memory.saved.as_ref(), Some(&authority)); + } + #[test] + fn substituted_legacy_credential_is_never_copied() { + let (mut authority, mut memory) = legacy(); + memory.legacy = Some("substitution".into()); + assert!(migrate(&mut authority, &mut memory).is_err()); + assert_eq!(memory.writes, 0); + assert!(memory.shared.is_none()); + } + #[test] + fn failed_write_keeps_legacy_credential_and_authority() { + let (mut authority, mut memory) = legacy(); + memory.fail_write = true; + assert!(migrate(&mut authority, &mut memory).is_err()); + assert_eq!(authority.backend(), Some(CredentialBackend::Keychain)); + assert_eq!(memory.legacy.as_deref(), Some("valid")); + memory.fail_write = false; + migrate(&mut authority, &mut memory).unwrap(); + } + #[test] + fn failed_verification_or_authority_commit_never_deletes_legacy() { + for corrupt in [false, true] { + let (mut authority, mut memory) = legacy(); + memory.corrupt_copy = corrupt; + memory.fail_save = !corrupt; + assert!(migrate(&mut authority, &mut memory).is_err()); + assert_eq!(authority.backend(), Some(CredentialBackend::Keychain)); + assert_eq!(memory.legacy.as_deref(), Some("valid")); + } + } + #[test] + fn interrupted_cleanup_uses_shared_authority_without_reimporting_legacy() { + let (mut authority, mut memory) = legacy(); + memory.fail_delete = true; + assert!(migrate(&mut authority, &mut memory).is_err()); + assert!(authority.has_pending_legacy_keychain_cleanup()); + assert_eq!(authority.backend(), Some(CredentialBackend::SharedKeychain)); + memory.legacy = Some("stale".into()); + memory.fail_delete = false; + migrate(&mut authority, &mut memory).unwrap(); + assert_eq!(memory.legacy_reads, 1); + assert_eq!(memory.shared.as_deref(), Some("valid")); + assert!(!authority.has_pending_legacy_keychain_cleanup()); + } + #[test] + fn revoked_and_file_credentials_never_touch_either_keychain() { + for mut authority in [ + CredentialAuthority::Revoked, + CredentialAuthority::active(CredentialBackend::EncryptedFileFallback, "valid"), + ] { + let mut memory = Memory::default(); + migrate(&mut authority, &mut memory).unwrap(); + assert_eq!(memory.legacy_reads, 0); + assert_eq!(memory.writes, 0); + assert!(memory.saved.is_none()); + } + } +} diff --git a/crates/lpm-auth/src/lib.rs b/crates/lpm-auth/src/lib.rs index 8f2d1b14..63377a1e 100644 --- a/crates/lpm-auth/src/lib.rs +++ b/crates/lpm-auth/src/lib.rs @@ -38,18 +38,13 @@ use std::path::{Path, PathBuf}; use credential_authority::{CredentialAuthority, CredentialBackend, CredentialKind}; #[cfg(target_os = "macos")] -use security_framework::passwords::{ - delete_generic_password as macos_delete_generic_password, - get_generic_password as macos_get_generic_password, - set_generic_password as macos_set_generic_password, -}; +use security_framework::base::Error as MacosSecurityError; #[cfg(target_os = "macos")] -use security_framework::{ - base::Error as MacosSecurityError, - item::{ItemClass, ItemSearchOptions, SearchResult}, -}; +mod macos_keychain; mod credential_authority; +#[cfg(any(target_os = "macos", test))] +mod keychain_migration; mod legacy_key; mod session; pub use session::{ @@ -65,9 +60,6 @@ const KEYCHAIN_ACCOUNT_PREFIX: &str = "auth-token"; const DISABLE_HOST_CLI_AUTH_ENV: &str = "LPM_DISABLE_HOST_CLI_AUTH"; -#[cfg(target_os = "macos")] -const ERR_SEC_ITEM_NOT_FOUND: i32 = -25300; - #[cfg(target_os = "macos")] const ERR_SEC_INTERACTION_NOT_ALLOWED: i32 = -25308; @@ -416,7 +408,7 @@ fn set_credential_with_keychain_writer_unlocked( credential_authority::set( registry, kind, - CredentialAuthority::active(CredentialBackend::Keychain, token), + CredentialAuthority::committed_keychain(token), ) .map_err(|error| { format!( @@ -934,6 +926,22 @@ fn get_stored_credential_with_backend_unlocked( CredentialKind::Refresh => scoped_refresh_account(registry), }; + let mut notice = Some(notice); + #[cfg(target_os = "macos")] + let authority = { + let mut authority = authority; + if !force_file_auth() + && let Some(record) = &mut authority + { + keychain_migration::prepare(registry, kind, &account, record, || { + if let Some(notice) = notice.take() { + notice(); + } + })?; + } + authority + }; + let resolved = resolve_stored_credential_from_backends( authority.as_ref(), || { @@ -948,7 +956,11 @@ fn get_stored_credential_with_backend_unlocked( KeychainCredentialProbe::Failed }) }, - notice, + || { + if let Some(notice) = notice.take() { + notice(); + } + }, || get_password_from_keychain_account(&account), || probe_token_from_file(&file_key), )?; @@ -988,7 +1000,7 @@ fn complete_pending_keychain_cleanup_with( credential_authority::set( registry, kind, - CredentialAuthority::active(CredentialBackend::Keychain, &credential.token), + CredentialAuthority::active(CredentialBackend::current_keychain(), &credential.token), )?; Ok(()) } @@ -2111,12 +2123,15 @@ fn set_password_in_keychain_account(account: &str, token: &str) -> Result<(), St } #[cfg(target_os = "macos")] -fn get_password_from_macos_keychain_native(service: &str, account: &str) -> MacosKeychainLookup { - let _lock = lpm_common::platform::macos_keychain_operation_lock(); - match macos_get_generic_password(service, account) { - Ok(password) => token_from_keychain_password(password) - .map_or(MacosKeychainLookup::NotFound, MacosKeychainLookup::Found), - Err(error) if error.code() == ERR_SEC_ITEM_NOT_FOUND => MacosKeychainLookup::NotFound, +fn macos_lookup( + service: &str, + account: &str, + scope: macos_keychain::Scope, + interactive: bool, +) -> MacosKeychainLookup { + match macos_keychain::read(service, account, scope, interactive) { + Ok(Some(token)) => MacosKeychainLookup::Found(token), + Ok(None) => MacosKeychainLookup::NotFound, Err(error) if error.code() == ERR_SEC_INTERACTION_NOT_ALLOWED => { MacosKeychainLookup::InteractionRequired } @@ -2124,52 +2139,20 @@ fn get_password_from_macos_keychain_native(service: &str, account: &str) -> Maco } } +#[cfg(target_os = "macos")] +fn get_password_from_macos_keychain_native(service: &str, account: &str) -> MacosKeychainLookup { + macos_lookup(service, account, macos_keychain::Scope::Shared, true) +} + #[cfg(target_os = "macos")] fn get_password_from_macos_keychain_noninteractive( service: &str, account: &str, ) -> MacosKeychainLookup { - use core_foundation::base::{CFType, TCFType}; - use objc2_local_authentication::LAContext; - - // SAFETY: `new` returns a retained `LAContext`, and the setter only changes - // whether Security.framework may present authentication UI for this query. - let context = unsafe { LAContext::new() }; - unsafe { - context.setInteractionNotAllowed(true); - } - // SAFETY: `kSecUseAuthenticationContext` explicitly accepts an LAContext - // object. The retained Objective-C object stays alive through `search`, - // and `wrap_under_get_rule` adds the ownership needed by the CFType wrapper. - let context_ptr = std::ptr::from_ref(&*context).cast::(); - let context_value = unsafe { CFType::wrap_under_get_rule(context_ptr) }; - - let _lock = lpm_common::platform::macos_keychain_operation_lock(); - let result = ItemSearchOptions::new() - .class(ItemClass::generic_password()) - .service(service) - .account(account) - .load_data(true) - .local_authentication_context(Some(context_value)) - .search(); - - match result { - Ok(results) => results - .into_iter() - .find_map(|result| match result { - SearchResult::Data(password) => token_from_keychain_password(password), - _ => None, - }) - .map_or(MacosKeychainLookup::NotFound, MacosKeychainLookup::Found), - Err(error) if error.code() == ERR_SEC_ITEM_NOT_FOUND => MacosKeychainLookup::NotFound, - Err(error) if error.code() == ERR_SEC_INTERACTION_NOT_ALLOWED => { - MacosKeychainLookup::InteractionRequired - } - Err(error) => MacosKeychainLookup::Failed(error), - } + macos_lookup(service, account, macos_keychain::Scope::Shared, false) } -#[cfg(target_os = "macos")] +#[cfg(all(target_os = "macos", test))] fn token_from_keychain_password(password: Vec) -> Option { String::from_utf8(password) .ok() @@ -2179,19 +2162,17 @@ fn token_from_keychain_password(password: Vec) -> Option { #[cfg(target_os = "macos")] fn set_password_in_macos_keychain(service: &str, account: &str, token: &str) -> Result<(), String> { - let _lock = lpm_common::platform::macos_keychain_operation_lock(); - macos_set_generic_password(service, account, token.as_bytes()) - .map_err(|error| format!("keychain write error: {error}")) + macos_keychain::write(service, account, token) + .map_err(|error| format!("shared Keychain write failed: {error}")) } #[cfg(target_os = "macos")] fn clear_password_from_macos_keychain(service: &str, account: &str) -> Result<(), String> { - let _lock = lpm_common::platform::macos_keychain_operation_lock(); - match macos_delete_generic_password(service, account) { - Ok(()) => Ok(()), - Err(error) if error.code() == ERR_SEC_ITEM_NOT_FOUND => Ok(()), - Err(error) => Err(format!("keychain delete error: {error}")), - } + let shared = macos_keychain::delete(service, account, macos_keychain::Scope::Shared); + let legacy = macos_keychain::delete(service, account, macos_keychain::Scope::Legacy); + shared + .and(legacy) + .map_err(|error| format!("Keychain delete failed: {error}")) } fn clear_password_from_keychain_account(account: &str) -> Result<(), String> { @@ -2981,8 +2962,7 @@ mod tests { assert_eq!(get_token_from_file(registry), None); assert_eq!( credential_authority::read(registry, CredentialKind::Access).unwrap(), - Some(CredentialAuthority::active( - CredentialBackend::Keychain, + Some(CredentialAuthority::committed_keychain( "new-keychain-token" )) ); @@ -3061,7 +3041,7 @@ mod tests { assert_eq!( credential_authority::read(registry, CredentialKind::Access).unwrap(), Some(CredentialAuthority::active( - CredentialBackend::Keychain, + CredentialBackend::current_keychain(), "matching-token" )) ); @@ -3694,7 +3674,7 @@ mod tests { if std::env::var("LPM_RUN_KEYCHAIN_TESTS").is_err() { panic!( "keychain integration test requires `LPM_RUN_KEYCHAIN_TESTS=1`. \ - Run via: `LPM_RUN_KEYCHAIN_TESTS=1 cargo test -p lpm-auth --lib tests::macos_auth_h4_write_round_trip -- --ignored --exact --test-threads=1`" + Run via: `LPM_RUN_KEYCHAIN_TESTS=1 cargo test -p lpm-auth --lib tests::macos_shared_auth_write_round_trip -- --ignored --exact --test-threads=1`" ); } } @@ -3717,11 +3697,9 @@ mod tests { #[cfg(target_os = "macos")] fn cleanup_keychain_item(service: &str, account: &str) { - let _ = std::process::Command::new("security") - .args(["delete-generic-password", "-s", service, "-a", account]) - .stdout(std::process::Stdio::null()) - .stderr(std::process::Stdio::null()) - .status(); + for scope in [macos_keychain::Scope::Shared, macos_keychain::Scope::Legacy] { + macos_keychain::delete(service, account, scope).expect("test Keychain cleanup failed"); + } } // `should_revalidate_when_marker_missing` and @@ -3808,7 +3786,7 @@ mod tests { #[cfg(target_os = "macos")] #[test] #[ignore = "macOS keychain integration; opt-in only and serial execution required"] - fn macos_auth_h4_write_round_trip() { + fn macos_shared_auth_write_round_trip() { require_keychain_opt_in(); with_test_keychain_service(|service| { @@ -3831,6 +3809,21 @@ mod tests { MacosKeychainLookup::Found(token) if token == "lpm_refresh_token" )); + if let Some(helper) = std::env::var_os("LPM_KEYCHAIN_INTEROP_HELPER") { + let status = std::process::Command::new(helper) + .args([service, &access_account]) + .status() + .expect("Swift Keychain helper failed to start"); + assert!( + status.success(), + "Swift must read the Rust credential and replace it" + ); + assert!(matches!( + get_password_from_macos_keychain_native(service, &access_account), + MacosKeychainLookup::Found(token) if token == "swift_interop_token" + )); + } + cleanup_keychain_item(service, &access_account); cleanup_keychain_item(service, &refresh_account); }); @@ -5738,7 +5731,9 @@ mod tests { /// surface a confusing message to the user). #[cfg(target_os = "macos")] #[test] + #[ignore = "requires a Developer ID signed test bundle with the CLI provisioning profile"] fn clear_password_from_keychain_account_treats_absent_as_ok() { + require_keychain_opt_in(); let account = format!( "lpm-test-absent-{}-{}", std::process::id(), @@ -5747,7 +5742,6 @@ mod tests { .unwrap() .as_nanos(), ); - // No prior write — the account is guaranteed absent. let result = clear_password_from_keychain_account(&account); assert!( result.is_ok(), diff --git a/crates/lpm-auth/src/macos_keychain.rs b/crates/lpm-auth/src/macos_keychain.rs new file mode 100644 index 00000000..205fd9fa --- /dev/null +++ b/crates/lpm-auth/src/macos_keychain.rs @@ -0,0 +1,217 @@ +use core_foundation::{ + base::{CFType, CFTypeRef, TCFType}, + boolean::CFBoolean, + data::CFData, + dictionary::CFDictionary, + string::{CFString, CFStringRef}, +}; +use security_framework::base::Error; +use security_framework_sys::{ + access_control::kSecAttrAccessibleWhenUnlockedThisDeviceOnly, + item::{ + kSecAttrAccessGroup, kSecAttrAccount, kSecAttrService, kSecAttrSynchronizable, kSecClass, + kSecClassGenericPassword, kSecReturnData, kSecUseAuthenticationContext, + kSecUseDataProtectionKeychain, kSecValueData, + }, + keychain_item::{SecItemAdd, SecItemCopyMatching, SecItemDelete, SecItemUpdate}, +}; + +#[link(name = "Security", kind = "framework")] +unsafe extern "C" { + static kSecAttrAccessible: CFStringRef; +} + +pub(super) const ACCESS_GROUP: &str = "823S8YKMRW.dev.lpm.vault.shared"; +const NOT_FOUND: i32 = -25300; +const DUPLICATE: i32 = -25299; + +#[derive(Clone, Copy)] +pub(super) enum Scope { + Shared, + Legacy, +} + +fn identity(service: &str, account: &str, scope: Scope) -> Vec<(CFString, CFType)> { + // SAFETY: Security.framework exports these keys and values as immortal CFStrings. + unsafe { + let mut pairs = vec![ + ( + CFString::wrap_under_get_rule(kSecClass), + CFString::wrap_under_get_rule(kSecClassGenericPassword).into_CFType(), + ), + ( + CFString::wrap_under_get_rule(kSecAttrService), + CFString::from(service).into_CFType(), + ), + ( + CFString::wrap_under_get_rule(kSecAttrAccount), + CFString::from(account).into_CFType(), + ), + ( + CFString::wrap_under_get_rule(kSecUseDataProtectionKeychain), + CFBoolean::from(matches!(scope, Scope::Shared)).into_CFType(), + ), + ]; + if matches!(scope, Scope::Shared) { + pairs.push(( + CFString::wrap_under_get_rule(kSecAttrAccessGroup), + CFString::from(ACCESS_GROUP).into_CFType(), + )); + pairs.push(( + CFString::wrap_under_get_rule(kSecAttrSynchronizable), + CFBoolean::from(false).into_CFType(), + )); + } + pairs + } +} + +pub(super) fn read( + service: &str, + account: &str, + scope: Scope, + interactive: bool, +) -> Result, Error> { + let mut pairs = identity(service, account, scope); + // SAFETY: The key is a framework-owned CFString and the value is retained by the query. + pairs.push(unsafe { + ( + CFString::wrap_under_get_rule(kSecReturnData), + CFBoolean::from(true).into_CFType(), + ) + }); + if !interactive { + // SAFETY: LAContext is retained until the query releases its CFType reference. + let context = unsafe { objc2_local_authentication::LAContext::new() }; + unsafe { context.setInteractionNotAllowed(true) }; + let pointer = std::ptr::from_ref(&*context).cast::(); + // SAFETY: kSecUseAuthenticationContext accepts an LAContext, and get-rule retains it. + pairs.push(unsafe { + ( + CFString::wrap_under_get_rule(kSecUseAuthenticationContext), + CFType::wrap_under_get_rule(pointer), + ) + }); + } + let query = CFDictionary::from_CFType_pairs(&pairs); + let mut result: CFTypeRef = std::ptr::null(); + let _lock = lpm_common::platform::macos_keychain_operation_lock(); + // SAFETY: The dictionary and output pointer remain valid for the synchronous call. + let status = unsafe { SecItemCopyMatching(query.as_concrete_TypeRef(), &mut result) }; + if status == NOT_FOUND { + return Ok(None); + } + if status != 0 { + return Err(Error::from_code(status)); + } + if result.is_null() { + return Err(Error::from_code(-26275)); + } + // SAFETY: A successful Copy call transfers one reference to the caller. + let value = unsafe { CFType::wrap_under_create_rule(result) }; + let data = value + .downcast_into::() + .ok_or_else(|| Error::from_code(-26275))?; + let token = std::str::from_utf8(data.bytes()) + .map_err(|_| Error::from_code(-26275))? + .trim(); + if token.is_empty() { + return Err(Error::from_code(-26275)); + } + Ok(Some(token.to_owned())) +} + +pub(super) fn write(service: &str, account: &str, token: &str) -> Result<(), Error> { + let mut identity = identity(service, account, Scope::Shared); + let query = CFDictionary::from_CFType_pairs(&identity); + // SAFETY: Framework constants are immortal; both owned attribute values survive the calls. + let attributes = unsafe { + vec![ + ( + CFString::wrap_under_get_rule(kSecValueData), + CFData::from_buffer(token.as_bytes()).into_CFType(), + ), + ( + CFString::wrap_under_get_rule(kSecAttrAccessible), + CFString::wrap_under_get_rule(kSecAttrAccessibleWhenUnlockedThisDeviceOnly) + .into_CFType(), + ), + ] + }; + let changes = CFDictionary::from_CFType_pairs(&attributes); + let _lock = lpm_common::platform::macos_keychain_operation_lock(); + // SAFETY: Both dictionaries remain alive during the synchronous call. + let status = + unsafe { SecItemUpdate(query.as_concrete_TypeRef(), changes.as_concrete_TypeRef()) }; + if status == 0 { + return Ok(()); + } + if status != NOT_FOUND { + return Err(Error::from_code(status)); + } + identity.extend(attributes); + let add = CFDictionary::from_CFType_pairs(&identity); + // SAFETY: The dictionary remains alive and no result object is requested. + let status = unsafe { SecItemAdd(add.as_concrete_TypeRef(), std::ptr::null_mut()) }; + let status = if status == DUPLICATE { + // SAFETY: A concurrent writer won the add; update only the same scoped identity. + unsafe { SecItemUpdate(query.as_concrete_TypeRef(), changes.as_concrete_TypeRef()) } + } else { + status + }; + if status == 0 { + Ok(()) + } else { + Err(Error::from_code(status)) + } +} + +pub(super) fn delete(service: &str, account: &str, scope: Scope) -> Result<(), Error> { + let query = CFDictionary::from_CFType_pairs(&identity(service, account, scope)); + let _lock = lpm_common::platform::macos_keychain_operation_lock(); + // SAFETY: The dictionary remains valid throughout the synchronous call. + match unsafe { SecItemDelete(query.as_concrete_TypeRef()) } { + 0 | NOT_FOUND => Ok(()), + status => Err(Error::from_code(status)), + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn auth_query_selects_the_shared_data_protection_group() { + let pairs = identity("lpm-cli", "test-account", Scope::Shared); + let description = format!("{pairs:?}"); + assert!(description.contains(ACCESS_GROUP)); + // SAFETY: Security.framework owns the immortal query key. + let key = unsafe { CFString::wrap_under_get_rule(kSecUseDataProtectionKeychain) }; + let pairs = identity("lpm-cli", "test-account", Scope::Shared); + let protected = pairs + .iter() + .find(|(name, _)| *name == key) + .unwrap() + .1 + .downcast::() + .unwrap(); + assert!(bool::from(protected)); + } + + #[test] + fn legacy_query_explicitly_excludes_the_shared_store() { + let description = format!("{:?}", identity("lpm-cli", "test-account", Scope::Legacy)); + assert!(!description.contains(ACCESS_GROUP)); + // SAFETY: Security.framework owns the immortal query key. + let key = unsafe { CFString::wrap_under_get_rule(kSecUseDataProtectionKeychain) }; + let pairs = identity("lpm-cli", "test-account", Scope::Legacy); + let protected = pairs + .iter() + .find(|(name, _)| *name == key) + .unwrap() + .1 + .downcast::() + .unwrap(); + assert!(!bool::from(protected)); + } +} From 0034c5550b94e722dd4765e5b67cadcd10b03c00 Mon Sep 17 00:00:00 2001 From: Tolga Ergin Date: Tue, 15 Sep 2026 21:13:40 +0100 Subject: [PATCH 2/2] Record shared auth migration validation --- SHARED_AUTH_KEYCHAIN_LEDGER.md | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) create mode 100644 SHARED_AUTH_KEYCHAIN_LEDGER.md diff --git a/SHARED_AUTH_KEYCHAIN_LEDGER.md b/SHARED_AUTH_KEYCHAIN_LEDGER.md new file mode 100644 index 00000000..8f9cb344 --- /dev/null +++ b/SHARED_AUTH_KEYCHAIN_LEDGER.md @@ -0,0 +1,21 @@ +# Shared auth Keychain finding + +| ID | Source | Category | Location | Claim | Evidence | Disposition | Coverage | Commit | PR status | +| --- | --- | --- | --- | --- | --- | --- | --- | --- | --- | +| AUTH-RUST | Primary agent | Security | `crates/lpm-auth` macOS auth queries | Queries omitted the shared access group and Data Protection selection. | A query-scope regression failed before the native query change. | Verified | Scoped native queries, six migration/recovery cases, signed Rust/Swift interoperability | `c98b86eb` | Concept PR | + +Totals: one direct finding, one verified and fixed, zero rejected, zero externally blocked findings, zero pending findings. Subagent findings received: zero. + +The companion Vault finding and release dependencies are recorded in lpm-dev/lpm-vault#24. + +## Validation + +- Rust 1.94.0 workspace build, formatting, and all-target clippy with warnings denied. +- Fast workspace nextest: 6,307 passed, nine skipped. +- CLI unit tests, serial: 5,131 passed, ten ignored. +- CLI integration nextest: 99 passed. +- Repository shell, installer, benchmark-helper, and npm wrapper/release checks passed. Native fish execution was skipped because fish is unavailable. +- Signed native Keychain round trip and absent-item deletion passed. +- A signed Swift helper read Rust's synthetic credential, replaced it, and Rust read the replacement. Test credentials were deleted. + +The native deletion test requires a signed bundle and provisioning profile. An unsigned test executable receives the expected missing-entitlement error, so this test runs in the signed opt-in suite.