kornelski / kornelski/rust-security-framework
SecKeychainItem::delete() discards the OSStatus, so a failed keychain deletion is indistinguishable from success
- Dominant language
- Rust
- Stars
- 304
- Forks
- 105
- PR merge metrics
- No merged PRs in 30d
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:
1. **here** — `delete()` returns `()`, `OSStatus` dropped
2. [`apple-native-keyring-store`](https://github.com/open-source-cooperative/apple-native-keyring-store) — `item.delete(); Ok(())` (filed separately)
3. [`@napi-rs/keyring`](https://github.com/Brooooooklyn/keyring-node) — `.is_ok()` on that `Ok` -> `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 to `SecItemDelete` here; 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.
Contributor guide
No contributing guide indexed for this repository
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.
Assessment
- Tech stack
- macos, rust
- Domain
- operating-systems, security
- Issue type
- Bug
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 78/100