kornelski / kornelski/rust-security-framework

SecKeychainItem::delete() discards the OSStatus, so a failed keychain deletion is indistinguishable from success

Open Beginner friendly
#256 0 comments 0 reactions 0 assignees View on GitHub
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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.