SecKeychainItem::delete() discards the OSStatus, so a failed keychain deletion is indistinguishable from success
Nobody has claimed this yet.
Assessment
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Newbie friendliness
- 78/100
- Issue type
- Bug
- Clarity
- Clearly specified
- Activity status
- Quiet
- Tech stack
- macos, rust
- Domain
- operating-systems, security
Research direction
Start in src/os/macos/passwords.rs at SecKeychainItem::delete(), then compare it with set_password() and the other deletion paths in src/item.rs and src/passwords.rs. Use the existing cvt helper from src/lib.rs and verify that deletion failures are returned to callers, choosing either the breaking signature change or the additive try_delete approach described in the issue.
Written by the indexing model from the issue text.
Description
Summary
SecKeychainItem::delete() in src/os/macos/passwords.rs calls SecKeychainItemDelete and discards the returned OSStatus, so a caller cannot tell a successful deletion from a failed one.
/// Delete this item from its keychain
#[inline]
pub fn delete(self) {
unsafe {
SecKeychainItemDelete(self.as_concrete_TypeRef());
}
}
(security-framework 3.7.0, src/os/macos/passwords.rs:79-85)
The underlying API does report failure — security-framework-sys declares it correctly:
#[cfg(target_os = "macos")]
pub fn SecKeychainItemDelete(itemRef: SecKeychainItemRef) -> OSStatus;
So the status is available and is being dropped on the floor.
Why this looks like an oversight rather than a decision
The method directly above it in the same impl SecKeychainItem block does exactly the right thing with the same pattern:
pub fn set_password(&mut self, password: &[u8]) -> Result<()> {
unsafe {
cvt(SecKeychainItemModifyAttributesAndData(
self.as_concrete_TypeRef(),
ptr::null(),
password.len() as u32,
password.as_ptr().cast(),
))?;
}
Ok(())
}
And the crate's other two deletion paths also check their status:
ItemSearchOptions::delete()—cvt(unsafe { SecItemDelete(...) })(src/item.rs:534)delete_generic_password_options()—cvt(unsafe { SecItemDelete(...) })(src/passwords.rs:77)
cvt is already available at src/lib.rs:51. This one method is the outlier.
Impact
This is a silent data-integrity problem for anything doing credential revocation, because the failure is invisible rather than noisy.
I found it from the other end. Tracing why a "log out" path in our application reported success while the secret stayed in the login Keychain, the discarded status turned out to be the first of three layers that erase it:
- here —
delete()returns(),OSStatusdropped apple-native-keyring-store—item.delete(); Ok(())(filed separately)@napi-rs/keyring—.is_ok()on thatOk->true
Net effect for a Node consumer on macOS: deleting a credential the OS refuses to delete resolves true. Not an error, not even false. Every downstream layer is doing something locally reasonable with information that was already destroyed at step 1, so no fix further up can recover it.
Suggested fix
/// Delete this item from its keychain
#[inline]
pub fn delete(self) -> Result<()> {
cvt(unsafe { SecKeychainItemDelete(self.as_concrete_TypeRef()) })
}
This is a breaking change to a public signature, so it presumably wants the next major. If that is a problem, an additive try_delete(self) -> Result<()> with delete() deprecated in its favour would also unblock downstream crates without forcing a major.
I am happy to send either as a PR — just say which shape you'd prefer.
Notes
- I have not attempted to judge whether the deprecated
SecKeychain*family should be migrated toSecItemDeletehere; that seems like a separate and larger question, and the one-line status check is useful regardless. - Verified against security-framework 3.7.0 / security-framework-sys 2.17.0 on aarch64-apple-darwin.
- Dominant language
- Rust
- Stars
- 304
- Forks
- 105
- PR merge metrics
- No merged PRs in 30d
Contributor guide
No contributing guide indexed for this repository
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
More from kornelski/rust-security-framework
-
Difficulty 2/5 1-3 hours Newbie friendliness 78/100
-
Difficulty 4/5 3-5 days Newbie friendliness 35/100
kornelski/rust-security-framework#251 · 4 comments ·
-
Difficulty 5/5 Over a week Newbie friendliness 30/100
kornelski/rust-security-framework#224 · 2 comments · 1 reaction ·
-
Difficulty 4/5 3-5 days Newbie friendliness 25/100
kornelski/rust-security-framework#205 · 5 comments ·
-
Difficulty 3/5 1-2 days Newbie friendliness 35/100
All issues in kornelski/rust-security-framework
Similar issues
-
Difficulty 2/5 1-3 hours Newbie friendliness 84/100
Eynzof/Hermes-CN-Desktop#610 ·
-
bug
Difficulty 2/5 1-3 hours Newbie friendliness 68/100
gitbutlerapp/gitbutler#15998 · 1 comment ·
-
bug triage:deciding
Difficulty 1/5 Under an hour Newbie friendliness 88/100
open-telemetry/otel-arrow#4132 ·
-
Difficulty 2/5 1-3 hours Newbie friendliness 88/100
-
Difficulty 2/5 1-3 hours Newbie friendliness 84/100