From c146acb18c39c6746a4ef5aa2f22955cdd7db0c2 Mon Sep 17 00:00:00 2001 From: Jay Gowdy Date: Thu, 21 May 2026 10:17:10 -0700 Subject: [PATCH] fix(apple): restore WhenUnlockedThisDeviceOnly for SE key access control MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #158 blanket-replaced all protection classes to AfterFirstUnlockThisDeviceOnly, including makeAccessControl() which sets the Secure Enclave key's access control at generation time. This broke CryptoKit's touchIDAuthenticationAllowableReuseDuration — every sign required Touch ID instead of caching biometric auth. Since the SE key's access control is immutable after creation, fixing this requires regenerating all affected keys. The keychain wrapping key (keychain_store) correctly uses AfterFirstUnlockThisDeviceOnly for sleep/wake survival. The SE key access control must use WhenUnlockedThisDeviceOnly for biometric caching. These are separate decisions with different requirements. Also adds AGENTS.md with protection class safety rules and updates CLAUDE.md to document the split, so this class of mistake is prevented in future changes. --- AGENTS.md | 69 ++++++++++++++++++++++ CLAUDE.md | 23 ++++++++ crates/enclaveapp-apple/swift/bridge.swift | 7 ++- 3 files changed, 98 insertions(+), 1 deletion(-) create mode 100644 AGENTS.md diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 0000000..2909c10 --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,69 @@ +# AGENTS.md + +Instructions for AI agents (Claude Code, Copilot, Cursor, etc.) working with libenclaveapp. + +## Protection Class Safety Rules + +**READ THIS BEFORE TOUCHING `bridge.swift`.** + +The macOS Apple backend has two distinct protection class use sites that +**MUST use different values**. Getting this wrong forces every downstream +user to regenerate all Secure Enclave keys — there is no migration path +because the SE key access control is baked into the SEP at creation time. + +### SE key access control (`makeAccessControl`) + +```swift +// MUST be WhenUnlockedThisDeviceOnly +SecAccessControlCreateWithFlags(nil, kSecAttrAccessibleWhenUnlockedThisDeviceOnly, flags, &error) +``` + +This is passed to `SecureEnclave.P256.Signing.PrivateKey(accessControl:)` at +key generation time. The protection class is **immutable** — once set, it +cannot be changed without deleting and regenerating the key. + +`WhenUnlockedThisDeviceOnly` is required because CryptoKit's +`touchIDAuthenticationAllowableReuseDuration` (biometric caching) only works +with this protection class. With `AfterFirstUnlockThisDeviceOnly`, the SEP +ignores the LAContext's cached authentication and prompts Touch ID on every +single sign operation. This was discovered in PR #158 — the blanket protection +class change broke biometric caching entirely, requiring full key regeneration +for all affected users. + +### Keychain wrapping key (`keychain_store`) + +```swift +// MUST be AfterFirstUnlockThisDeviceOnly +SecAccessControlCreateWithFlags(nil, kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly, ...) +``` + +The wrapping key is stored as a `kSecClassGenericPassword` keychain item. +`AfterFirstUnlockThisDeviceOnly` keeps the keybag class key in memory from +first unlock until reboot, so background agents (like sshenc-agent) can +access wrapping keys after sleep/wake without requiring the screen to be +unlocked. `WhenUnlockedThisDeviceOnly` purges the class key on device +lock/sleep, causing `-25308 (errSecInteractionNotAllowed)` failures. + +### Rules + +1. **NEVER do a blanket find-and-replace of protection class constants.** These + two sites have different requirements for different reasons. +2. **NEVER change `makeAccessControl`'s protection class** unless you have + verified with Apple documentation that biometric caching + (`touchIDAuthenticationAllowableReuseDuration`) works with the new class. + The cost of getting this wrong is catastrophic — every user must regenerate + all keys. +3. **Test biometric caching after any change to `bridge.swift`**: first sign + should trigger Touch ID (~2-3s), second sign within the cache window should + complete in <50ms. If every sign takes 2-3s, biometric caching is broken. +4. When changing keychain-related code, consider both sites independently and + document which one you are changing and why. + +## Build Safety (macOS) + +See the consuming app's AGENTS.md (e.g., sshenc/AGENTS.md) for rules about +never running unsigned development builds as agents on macOS. + +## Commits + +Do not add Co-Authored-By lines for Claude Code in commit messages. diff --git a/CLAUDE.md b/CLAUDE.md index e92f911..6ad3882 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -53,6 +53,29 @@ Rust workspace under `crates/`: - **enclaveapp-build-support** — Shared build.rs helper for Windows PE resource compilation. - **enclaveapp-test-support** — `MockKeyBackend` implementing all three traits with deterministic in-memory operations. XOR-based mock crypto for testing control flow without hardware. +### macOS Keychain Protection Classes — CRITICAL + +There are **two separate protection class decisions** in the Apple backend, and +they **MUST use different values**. Mixing them up causes catastrophic user +impact (broken biometric caching → Touch ID on every sign → key regeneration +required, because the SE key's access control is immutable after creation). + +| What | Function | Protection class | Why | +|------|----------|-----------------|-----| +| **SE key access control** | `makeAccessControl()` in `bridge.swift` | `kSecAttrAccessibleWhenUnlockedThisDeviceOnly` | Required for `touchIDAuthenticationAllowableReuseDuration` biometric caching in CryptoKit. Using `AfterFirstUnlock` silently breaks LAContext reuse — every sign requires Touch ID. | +| **Keychain wrapping key** | `keychain_store()` in `bridge.swift` | `kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly` | Must survive sleep/wake. `WhenUnlocked` purges the keybag class key on device lock, making the wrapping key inaccessible to background agents. | + +**NEVER do a blanket find-and-replace of protection class constants.** These two +use sites have different requirements for different reasons. If you change one, +verify you are not changing the other. + +**The SE key access control is immutable.** Once a key is generated with a given +`SecAccessControl`, that access control is baked into the Secure Enclave +Processor. There is no migration path — the key must be deleted and +regenerated. This means getting `makeAccessControl()` wrong forces every user +to regenerate all their keys. This has already happened once (PR #158) and +must never happen again. + ### Key Patterns - `EnclaveKeyManager` trait is the base — `EnclaveSigner` and `EnclaveEncryptor` extend it. diff --git a/crates/enclaveapp-apple/swift/bridge.swift b/crates/enclaveapp-apple/swift/bridge.swift index 3be65b0..a3e9976 100644 --- a/crates/enclaveapp-apple/swift/bridge.swift +++ b/crates/enclaveapp-apple/swift/bridge.swift @@ -276,9 +276,14 @@ func makeAccessControl(_ authPolicy: Int32) -> SecAccessControl? { setLastError("unsupported auth_policy value \(authPolicy)") return nil } + // SE key access control uses WhenUnlockedThisDeviceOnly — this is + // required for touchIDAuthenticationAllowableReuseDuration biometric + // caching to work in CryptoKit. The keychain wrapping key uses + // AfterFirstUnlockThisDeviceOnly separately (in keychain_store) so + // it survives sleep/wake. var error: Unmanaged? let ac = SecAccessControlCreateWithFlags( - nil, kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly, flags, &error + nil, kSecAttrAccessibleWhenUnlockedThisDeviceOnly, flags, &error ) if ac == nil { let desc = error.map { $0.takeRetainedValue().localizedDescription } ?? "unknown"