From 2c9632866620cfe376e6a8159ec969dbe9d1dc6c Mon Sep 17 00:00:00 2001 From: Klim Tsoutsman Date: Sat, 31 Aug 2024 10:43:53 +1000 Subject: [PATCH] Implement review Signed-off-by: Klim Tsoutsman --- security-framework/src/item.rs | 127 +++++++++++++++++++-------------- 1 file changed, 73 insertions(+), 54 deletions(-) diff --git a/security-framework/src/item.rs b/security-framework/src/item.rs index 71395b58..fe9a75d3 100644 --- a/security-framework/src/item.rs +++ b/security-framework/src/item.rs @@ -32,35 +32,35 @@ pub struct ItemClass(CFStringRef); impl ItemClass { /// Look for `SecKeychainItem`s corresponding to generic passwords. - #[inline(always)] + #[inline] #[must_use] pub fn generic_password() -> Self { unsafe { Self(kSecClassGenericPassword) } } /// Look for `SecKeychainItem`s corresponding to internet passwords. - #[inline(always)] + #[inline] #[must_use] pub fn internet_password() -> Self { unsafe { Self(kSecClassInternetPassword) } } /// Look for `SecCertificate`s. - #[inline(always)] + #[inline] #[must_use] pub fn certificate() -> Self { unsafe { Self(kSecClassCertificate) } } /// Look for `SecKey`s. - #[inline(always)] + #[inline] #[must_use] pub fn key() -> Self { unsafe { Self(kSecClassKey) } } /// Look for `SecIdentity`s. - #[inline(always)] + #[inline] #[must_use] pub fn identity() -> Self { unsafe { Self(kSecClassIdentity) } @@ -73,17 +73,17 @@ pub struct KeyClass(CFStringRef); impl KeyClass { /// `kSecAttrKeyClassPublic` - #[inline(always)] + #[inline] #[must_use] pub fn public() -> Self { unsafe { Self(kSecAttrKeyClassPublic) } } /// `kSecAttrKeyClassPrivate` - #[inline(always)] + #[inline] #[must_use] pub fn private() -> Self { unsafe { Self(kSecAttrKeyClassPrivate) } } /// `kSecAttrKeyClassSymmetric` - #[inline(always)] + #[inline] #[must_use] pub fn symmetric() -> Self { unsafe { Self(kSecAttrKeyClassSymmetric) } } @@ -151,21 +151,21 @@ impl crate::ItemSearchOptionsInternals for ItemSearchOptions { impl ItemSearchOptions { /// Creates a new builder with default options. - #[inline(always)] + #[inline] #[must_use] pub fn new() -> Self { Self::default() } /// Search only for items of the specified class. - #[inline(always)] + #[inline] pub fn class(&mut self, class: ItemClass) -> &mut Self { self.class = Some(class); self } /// Whether search for an item should be case insensitive or not. - #[inline(always)] + #[inline] pub fn case_insensitive(&mut self, case_insensitive: Option) -> &mut Self { self.case_insensitive = case_insensitive; self @@ -173,7 +173,7 @@ impl ItemSearchOptions { /// Search only for keys of the specified class. Also sets self.class to /// `ItemClass::key()`. - #[inline(always)] + #[inline] pub fn key_class(&mut self, key_class: KeyClass) -> &mut Self { self.class(ItemClass::key()); self.key_class = Some(key_class); @@ -182,7 +182,7 @@ impl ItemSearchOptions { /// Load Security Framework objects (`SecCertificate`, `SecKey`, etc) for /// the results. - #[inline(always)] + #[inline] pub fn load_refs(&mut self, load_refs: bool) -> &mut Self { self.load_refs = load_refs; self @@ -190,7 +190,7 @@ impl ItemSearchOptions { /// Load Security Framework object attributes for /// the results. - #[inline(always)] + #[inline] pub fn load_attributes(&mut self, load_attributes: bool) -> &mut Self { self.load_attributes = load_attributes; self @@ -198,7 +198,7 @@ impl ItemSearchOptions { /// Load Security Framework objects data for /// the results. - #[inline(always)] + #[inline] pub fn load_data(&mut self, load_data: bool) -> &mut Self { self.load_data = load_data; self @@ -207,55 +207,56 @@ impl ItemSearchOptions { /// Limit the number of search results. /// /// If this is not called, the default limit is 1. - #[inline(always)] + #[inline] pub fn limit>(&mut self, limit: T) -> &mut Self { self.limit = Some(limit.into()); self } /// Search for an item with the given label. - #[inline(always)] + #[inline] pub fn label(&mut self, label: &str) -> &mut Self { self.label = Some(CFString::new(label)); self } /// Whether untrusted certificates should be returned. - #[inline(always)] + #[inline] pub fn trusted_only(&mut self, trusted_only: Option) -> &mut Self { self.trusted_only = trusted_only; self } /// Search for an item with the given service. - #[inline(always)] + #[inline] pub fn service(&mut self, service: &str) -> &mut Self { self.service = Some(CFString::new(service)); self } /// Search for an item with exactly the given subject. - #[inline(always)] + #[inline] pub fn subject(&mut self, subject: &str) -> &mut Self { self.subject = Some(CFString::new(subject)); self } /// Search for an item with the given account. - #[inline(always)] + #[inline] pub fn account(&mut self, account: &str) -> &mut Self { self.account = Some(CFString::new(account)); self } /// Search for an item with a specific access group. + #[inline] pub fn access_group(&mut self, access_group: &str) -> &mut Self { self.access_group = Some(CFString::new(access_group)); self } /// Sets `kSecAttrAccessGroup` to `kSecAttrAccessGroupToken` - #[inline(always)] + #[inline] pub fn access_group_token(&mut self) -> &mut Self { self.access_group = unsafe { Some(CFString::wrap_under_get_rule(kSecAttrAccessGroupToken)) }; self @@ -266,7 +267,7 @@ impl ItemSearchOptions { /// This is only compatible with [`ItemClass::certificate`], to search for /// a key by public key hash use [`ItemSearchOptions::application_label`] /// instead. - #[inline(always)] + #[inline] pub fn pub_key_hash(&mut self, pub_key_hash: &[u8]) -> &mut Self { self.pub_key_hash = Some(CFData::from_buffer(pub_key_hash)); self @@ -277,15 +278,15 @@ impl ItemSearchOptions { /// This is only compatible with [`ItemClass::key`], to search for a /// certificate by the public key hash use [`ItemSearchOptions::pub_key_hash`] /// instead. - #[inline(always)] + #[inline] pub fn application_label(&mut self, app_label: &[u8]) -> &mut Self { self.app_label = Some(CFData::from_buffer(app_label)); self } /// Populates a `CFDictionary` to be passed to `update_item` or `delete_item`. - #[inline(always)] - pub fn to_dictionary(&self) -> CFDictionary { + #[inline] + fn to_dictionary(&self) -> CFDictionary { unsafe { let mut params = CFMutableDictionary::from_CFType_pairs(&[]); @@ -441,6 +442,13 @@ impl ItemSearchOptions { Ok(items) } } + + /// Deletes objects matching the search options. + /// + /// Translates to `SecItemDelete`. + pub fn delete(&self) -> Result<()> { + cvt(unsafe { SecItemDelete(self.to_dictionary().as_concrete_TypeRef()) }) + } } unsafe fn get_item(item: CFTypeRef) -> SearchResult { @@ -578,8 +586,7 @@ impl SearchResult { /// Builder-pattern struct for specifying options for `add_item` (`SecAddItem` /// wrapper). /// -/// When finished populating options, call `to_dictionary()` and pass the -/// resulting `CFDictionary` to `add_item`. +/// When finished populating options call [`Self::add`]. pub struct ItemAddOptions { /// The value (by ref or data) of the item to add, required. pub value: ItemAddValue, @@ -642,6 +649,7 @@ impl ItemAddOptions { self } /// Populates a `CFDictionary` to be passed to `add_item`. + #[deprecated(since = "3.0.0", note = "use `ItemAddOptions::add` instead")] pub fn to_dictionary(&self) -> CFDictionary { let mut dict = CFMutableDictionary::from_CFType_pairs(&[]); @@ -697,6 +705,13 @@ impl ItemAddOptions { dict.to_immutable() } + /// Adds the item to the keychain. + /// + /// Translates to `SecItemAdd`. + pub fn add(&self) -> Result<()> { + #[allow(deprecated)] + cvt(unsafe { SecItemAdd(self.to_dictionary().as_concrete_TypeRef(), std::ptr::null_mut()) }) + } } /// Value of an item to add to the keychain. @@ -744,12 +759,11 @@ impl AddRef { /// Builder-pattern struct for specifying options for `update_item` (`SecUpdateItem` /// wrapper). /// -/// When finished populating options, call `to_dictionary()` and pass the -/// resulting `CFDictionary` to `update_item`. +/// When finished populating options call [`update_item`]. #[derive(Default)] pub struct ItemUpdateOptions { /// Optional value (by ref or data) of the item to update. - pub value: Option, + pub value: Option, /// Optional kSecAttrAccount attribute. pub account_name: Option, /// Optional kSecAttrAccessGroup attribute. @@ -765,11 +779,9 @@ pub struct ItemUpdateOptions { /// Optional keychain location. pub location: Option, /// Optional kSecClass. - /// - /// This overrides the class provided by `value`. /// - /// If set to `None`, `value`'s class is ignored, and not added to the `CFDictionary`. - pub class: Option>, + /// Overwrites `value`'s class if set. + pub class: Option, } impl ItemUpdateOptions { @@ -780,12 +792,12 @@ impl ItemUpdateOptions { } /// Specifies the value of the item. - pub fn set_value(&mut self, value: ItemAddValue) -> &mut Self { + pub fn set_value(&mut self, value: ItemUpdateValue) -> &mut Self { self.value = Some(value); self } /// Specifies the `kSecClass` attribute. - pub fn set_class(&mut self, class: Option) -> &mut Self { + pub fn set_class(&mut self, class: ItemClass) -> &mut Self { self.class = Some(class); self } @@ -825,27 +837,27 @@ impl ItemUpdateOptions { self } /// Populates a `CFDictionary` to be passed to `update_item`. - pub fn to_dictionary(&self) -> CFDictionary { + fn to_dictionary(&self) -> CFDictionary { let mut dict = CFMutableDictionary::from_CFType_pairs(&[]); if let Some(ref value) = self.value { let class_opt = match value { - ItemAddValue::Ref(ref_) => ref_.class(), - ItemAddValue::Data { class, .. } => Some(*class), + ItemUpdateValue::Ref(ref_) => ref_.class(), + ItemUpdateValue::Data(_) => None, }; - // We only write value's class if `set_class` wasn't called. + // `self.class` overwrites `value`'s class if set. if self.class.is_none() { if let Some(class) = class_opt { dict.add(&unsafe { kSecClass }.to_void(), &class.0.to_void()); } } let value_pair = match value { - ItemAddValue::Ref(ref_) => (unsafe { kSecValueRef }.to_void(), ref_.ref_()), - ItemAddValue::Data { data, .. } => (unsafe { kSecValueData }.to_void(), data.to_void()), + ItemUpdateValue::Ref(ref_) => (unsafe { kSecValueRef }.to_void(), ref_.ref_()), + ItemUpdateValue::Data(data) => (unsafe { kSecValueData }.to_void(), data.to_void()), }; dict.add(&value_pair.0, &value_pair.1); } - if let Some(Some(class)) = self.class { + if let Some(class) = self.class { dict.add(&unsafe { kSecClass }.to_void(), &class.0.to_void()); } if let Some(location) = &self.location { @@ -888,6 +900,17 @@ impl ItemUpdateOptions { } } +/// Value of an item to update in the keychain. +pub enum ItemUpdateValue { + /// Pass item by Ref (kSecValueRef) + Ref(AddRef), + /// Pass item by Data (kSecValueData) + /// + /// Note that if the [`ItemClass`] of the updated data is different to the original data + /// stored in the keychain, it should be specified using [`ItemUpdateOptions::set_class`]. + Data(CFData), +} + /// Which keychain to add an item to. /// /// @@ -913,24 +936,20 @@ pub enum Location { /// Translates to `SecItemAdd`. Use `ItemAddOptions` to build an `add_params` /// `CFDictionary`. +#[deprecated(since = "3.0.0", note = "use `ItemAddOptions::add` instead")] +#[allow(deprecated)] pub fn add_item(add_params: CFDictionary) -> Result<()> { cvt(unsafe { SecItemAdd(add_params.as_concrete_TypeRef(), std::ptr::null_mut()) }) } -/// Translates to `SecItemUpdate`. Use `ItemSearchOptions` to build a `search_params` `CFDictionary` -/// and `ItemUpdateOptions` to build an `update_params` `CFDictionary`. -pub fn update_item(search_params: CFDictionary, update_params: CFDictionary) -> Result<()> { +/// Translates to `SecItemUpdate`. +pub fn update_item(search_params: &ItemSearchOptions, update_params: &ItemUpdateOptions) -> Result<()> { cvt(unsafe { SecItemUpdate( - search_params.as_concrete_TypeRef(), update_params.as_concrete_TypeRef() + search_params.to_dictionary().as_concrete_TypeRef(), + update_params.to_dictionary().as_concrete_TypeRef() )}) } -/// Translates to `SecItemDelete`. Use `ItemSearchOptions` to build a `search_params` -/// `CFDictionary`. -pub fn delete_item(search_params: CFDictionary) -> Result<()> { - cvt(unsafe { SecItemDelete(search_params.as_concrete_TypeRef()) }) -} - #[cfg(test)] mod test { use super::*;