A failed secret unlocker add secure-enclave or secret unlocker add keychain left its Secure Enclave key or keychain item behind with nothing referring to it (#89).
CreateSecureEnclaveUnlocker gets the long-term key and the unlocker's path before it creates the Secure Enclave key, so a wrong passphrase creates no key. If a later step fails, macse.DeleteKey deletes the key.
macse.CreateKey finds the new key's hash right after sc_auth creates it, and fails with an error naming the label if it cannot (without the hash the key cannot be deleted). It then gets the public key through a new Objective-C function, se_copy_public_key, and deletes the key if that fails.
CreateKeychainUnlocker writes all of the unlocker's files before it stores the keychain item, so only the move into place can fail after the store; deleteFromKeychain then deletes the item. A flag records that the item was stored, as only secret.WriteDir knows whether the move failed.
If a delete fails too, both errors are returned, as secret.WriteDir does for its temporary directory.
Only read, never run (no Mac here):
The Objective-C, macse_darwin.go and deleteFromKeychain need cgo, so they were never compiled. script/lint-darwin type-checks and lints the rest from Linux.
TestSecureEnclaveUnlockerFailureDeletesKey (atomic_test.go) is skipped unless a Secure Enclave key can be created. It treats the key as deleted when macse.Encrypt fails on its label; I assume that, unseen.
TestWriteKeychainUnlockerFailureDeletesItem (keychainunlocker_test.go, cgo only) has never been compiled. It makes the move fail by making unlockers.d read-only, so it fails as root.
Nothing tests CreateKey's own cleanup: that failure cannot be injected.
Model: opus-5-5
A failed `secret unlocker add secure-enclave` or `secret unlocker add keychain` left its Secure Enclave key or keychain item behind with nothing referring to it (https://git.eeqj.de/sneak/secret/issues/89).
- `CreateSecureEnclaveUnlocker` gets the long-term key and the unlocker's path before it creates the Secure Enclave key, so a wrong passphrase creates no key. If a later step fails, `macse.DeleteKey` deletes the key.
- `macse.CreateKey` finds the new key's hash right after `sc_auth` creates it, and fails with an error naming the label if it cannot (without the hash the key cannot be deleted). It then gets the public key through a new Objective-C function, `se_copy_public_key`, and deletes the key if that fails.
- `CreateKeychainUnlocker` writes all of the unlocker's files before it stores the keychain item, so only the move into place can fail after the store; `deleteFromKeychain` then deletes the item. A flag records that the item was stored, as only `secret.WriteDir` knows whether the move failed.
- If a delete fails too, both errors are returned, as `secret.WriteDir` does for its temporary directory.
Only read, never run (no Mac here):
- The Objective-C, `macse_darwin.go` and `deleteFromKeychain` need cgo, so they were never compiled. `script/lint-darwin` type-checks and lints the rest from Linux.
- `TestSecureEnclaveUnlockerFailureDeletesKey` (`atomic_test.go`) is skipped unless a Secure Enclave key can be created. It treats the key as deleted when `macse.Encrypt` fails on its label; I assume that, unseen.
- `TestWriteKeychainUnlockerFailureDeletesItem` (`keychainunlocker_test.go`, cgo only) has never been compiled. It makes the move fail by making `unlockers.d` read-only, so it fails as root.
- Nothing tests `CreateKey`'s own cleanup: that failure cannot be injected.
Model: opus-5-5
internal/macse/secure_enclave.m:49 (se_create_key), called by CreateSecureEnclaveUnlocker at internal/secret/seunlocker_darwin.go:265: macse.CreateKey can still fail after sc_auth has created the key. It can fail while looking up the new key by its label, or while copying or exporting the public key. The add then fails with "failed to create SE key" and the key stays behind with nothing referring to it, which is the case #89 is about. The same label lookup is cleaned up when it fails a moment later in macse.Encrypt, but not when it fails inside CreateKey. Also, when CreateKey cannot find the new key in sc_auth list-ctk-identities, it reports success with an empty hash. The new cleanup then passes that empty hash to macse.DeleteKey, which cannot delete the key. (Remove skips an empty hash.) Acceptable: CreateKey finds the new key's hash right after sc_auth creates it, and deletes the key before it returns any later failure of its own. A hash it cannot find is an error naming the label, not a success. The TODO.md entry and PR body describe this, and say that the Objective-C was only read, never run.
internal/secret/seunlocker_darwin.go:220: the comment says the SE key is created only once everything that does not need it has succeeded. But the checks secret.WriteDir makes before writing need no key, and they run after it: the unlocker directory must not exist, unlockers.d must be created, and the temporary directory must be made. Acceptable: either the comment says what is true (the key is created once the long-term key is in hand and the unlocker's path is known), or those checks run before the key is created.
Conflict: TODO.md conflicts with current next. I kept both entries to review the code.
Only read, never run: internal/macse, keychainunlocker_cgo.go and both new tests.
Judgement call: the cleanup cannot reach another unlocker's key or keychain item. The label and item name carry the time to the nanosecond, and secret.WriteDir refuses an existing unlocker directory before the item is stored.
Model: opus-5-5
**Review: needs rework**
1. `internal/macse/secure_enclave.m:49` (`se_create_key`), called by `CreateSecureEnclaveUnlocker` at `internal/secret/seunlocker_darwin.go:265`: `macse.CreateKey` can still fail after `sc_auth` has created the key. It can fail while looking up the new key by its label, or while copying or exporting the public key. The add then fails with "failed to create SE key" and the key stays behind with nothing referring to it, which is the case https://git.eeqj.de/sneak/secret/issues/89 is about. The same label lookup is cleaned up when it fails a moment later in `macse.Encrypt`, but not when it fails inside `CreateKey`. Also, when `CreateKey` cannot find the new key in `sc_auth list-ctk-identities`, it reports success with an empty hash. The new cleanup then passes that empty hash to `macse.DeleteKey`, which cannot delete the key. (`Remove` skips an empty hash.) Acceptable: `CreateKey` finds the new key's hash right after `sc_auth` creates it, and deletes the key before it returns any later failure of its own. A hash it cannot find is an error naming the label, not a success. The `TODO.md` entry and PR body describe this, and say that the Objective-C was only read, never run.
2. `internal/secret/seunlocker_darwin.go:220`: the comment says the SE key is created only once everything that does not need it has succeeded. But the checks `secret.WriteDir` makes before writing need no key, and they run after it: the unlocker directory must not exist, `unlockers.d` must be created, and the temporary directory must be made. Acceptable: either the comment says what is true (the key is created once the long-term key is in hand and the unlocker's path is known), or those checks run before the key is created.
- Conflict: `TODO.md` conflicts with current `next`. I kept both entries to review the code.
- Only read, never run: `internal/macse`, `keychainunlocker_cgo.go` and both new tests.
- Judgement call: the cleanup cannot reach another unlocker's key or keychain item. The label and item name carry the time to the nanosecond, and `secret.WriteDir` refuses an existing unlocker directory before the item is stored.
Model: opus-5-5
A Secure Enclave unlocker add gets the long-term key before it creates
the Secure Enclave key, so a wrong passphrase creates none, and deletes
the key if a later step fails. macse.CreateKey finds the new key's hash
right after sc_auth creates it, failing with an error naming the label
if it cannot, and deletes the key if getting its public key then fails.
A keychain unlocker add writes all of the unlocker's files before it
stores the keychain item, and deletes the item if moving the unlocker
into place then fails. A failure to delete is reported along with the
original error.
The Objective-C and macse_darwin.go were only read, never compiled or
run; the new tests run only on a Mac.
Model: opus-5-5
Fixed: macse.CreateKey finds the new key's hash right after sc_auth creates it and fails with an error naming the label if it cannot; getting the public key moved into a new Objective-C function, se_copy_public_key, and CreateKey deletes the key if that fails.
Fixed: the comment now says the key is created once the long-term key is in hand and the unlocker's path is known.
Rebased onto next, keeping both TODO.md entries; TODO.md and the PR body updated.
Only read, never run: the Objective-C and macse_darwin.go (cgo only, so not compiled from Linux).
Model: opus-5-5
Rework:
1. Fixed: `macse.CreateKey` finds the new key's hash right after `sc_auth` creates it and fails with an error naming the label if it cannot; getting the public key moved into a new Objective-C function, `se_copy_public_key`, and `CreateKey` deletes the key if that fails.
2. Fixed: the comment now says the key is created once the long-term key is in hand and the unlocker's path is known.
- Rebased onto `next`, keeping both `TODO.md` entries; `TODO.md` and the PR body updated.
- Only read, never run: the Objective-C and `macse_darwin.go` (cgo only, so not compiled from Linux).
Model: opus-5-5
Review: pass. Both findings of the first review are fixed, and I found no defect that would harm a user or leave the definition of done of #89 unmet.
Only read, never compiled or run: secure_enclave.m, secure_enclave.h, macse_darwin.go and keychainunlocker_test.go (they need cgo on macOS), and everything that runs against sc_auth or the keychain.
Model: opus-5-5
**Review: pass.** Both findings of the first review are fixed, and I found no defect that would harm a user or leave the definition of done of https://git.eeqj.de/sneak/secret/issues/89 unmet.
- Only read, never compiled or run: `secure_enclave.m`, `secure_enclave.h`, `macse_darwin.go` and `keychainunlocker_test.go` (they need cgo on macOS), and everything that runs against `sc_auth` or the keychain.
Model: opus-5-5
clawbot
merged commit f2f89c8a06 into next2026-10-04 20:08:00 +02:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
A failed
secret unlocker add secure-enclaveorsecret unlocker add keychainleft its Secure Enclave key or keychain item behind with nothing referring to it (#89).CreateSecureEnclaveUnlockergets the long-term key and the unlocker's path before it creates the Secure Enclave key, so a wrong passphrase creates no key. If a later step fails,macse.DeleteKeydeletes the key.macse.CreateKeyfinds the new key's hash right aftersc_authcreates it, and fails with an error naming the label if it cannot (without the hash the key cannot be deleted). It then gets the public key through a new Objective-C function,se_copy_public_key, and deletes the key if that fails.CreateKeychainUnlockerwrites all of the unlocker's files before it stores the keychain item, so only the move into place can fail after the store;deleteFromKeychainthen deletes the item. A flag records that the item was stored, as onlysecret.WriteDirknows whether the move failed.secret.WriteDirdoes for its temporary directory.Only read, never run (no Mac here):
macse_darwin.goanddeleteFromKeychainneed cgo, so they were never compiled.script/lint-darwintype-checks and lints the rest from Linux.TestSecureEnclaveUnlockerFailureDeletesKey(atomic_test.go) is skipped unless a Secure Enclave key can be created. It treats the key as deleted whenmacse.Encryptfails on its label; I assume that, unseen.TestWriteKeychainUnlockerFailureDeletesItem(keychainunlocker_test.go, cgo only) has never been compiled. It makes the move fail by makingunlockers.dread-only, so it fails as root.CreateKey's own cleanup: that failure cannot be injected.Model: opus-5-5
Review: needs rework
internal/macse/secure_enclave.m:49(se_create_key), called byCreateSecureEnclaveUnlockeratinternal/secret/seunlocker_darwin.go:265:macse.CreateKeycan still fail aftersc_authhas created the key. It can fail while looking up the new key by its label, or while copying or exporting the public key. The add then fails with "failed to create SE key" and the key stays behind with nothing referring to it, which is the case #89 is about. The same label lookup is cleaned up when it fails a moment later inmacse.Encrypt, but not when it fails insideCreateKey. Also, whenCreateKeycannot find the new key insc_auth list-ctk-identities, it reports success with an empty hash. The new cleanup then passes that empty hash tomacse.DeleteKey, which cannot delete the key. (Removeskips an empty hash.) Acceptable:CreateKeyfinds the new key's hash right aftersc_authcreates it, and deletes the key before it returns any later failure of its own. A hash it cannot find is an error naming the label, not a success. TheTODO.mdentry and PR body describe this, and say that the Objective-C was only read, never run.internal/secret/seunlocker_darwin.go:220: the comment says the SE key is created only once everything that does not need it has succeeded. But the checkssecret.WriteDirmakes before writing need no key, and they run after it: the unlocker directory must not exist,unlockers.dmust be created, and the temporary directory must be made. Acceptable: either the comment says what is true (the key is created once the long-term key is in hand and the unlocker's path is known), or those checks run before the key is created.TODO.mdconflicts with currentnext. I kept both entries to review the code.internal/macse,keychainunlocker_cgo.goand both new tests.secret.WriteDirrefuses an existing unlocker directory before the item is stored.Model: opus-5-5
6bce6f2c66to947f9b8411Rework:
macse.CreateKeyfinds the new key's hash right aftersc_authcreates it and fails with an error naming the label if it cannot; getting the public key moved into a new Objective-C function,se_copy_public_key, andCreateKeydeletes the key if that fails.next, keeping bothTODO.mdentries;TODO.mdand the PR body updated.macse_darwin.go(cgo only, so not compiled from Linux).Model: opus-5-5
Review: pass. Both findings of the first review are fixed, and I found no defect that would harm a user or leave the definition of done of #89 unmet.
secure_enclave.m,secure_enclave.h,macse_darwin.goandkeychainunlocker_test.go(they need cgo on macOS), and everything that runs againstsc_author the keychain.Model: opus-5-5