sneak/secret
1
0
Fork 0
Code Issues Pull Requests 1 Actions Packages Projects Releases Wiki Activity

secure-enclave-unlocker #24

Merged
sneak merged 7 commits from secure-enclave-unlocker into main 2026-03-14 07:36:28 +01:00
Conversation 5 Commits 7 Files Changed 20 +1458 -82
sneak commented 2026-03-11 02:34:41 +01:00
Owner
Copy Link
No description provided.
sneak added 2 commits 2026-03-11 02:34:41 +01:00
Add Secure Enclave unlocker for hardware-backed secret protection 4adeeae1db 
Adds a new "secure-enclave" unlocker type that stores the vault's
long-term private key encrypted by a non-exportable P-256 key held
in the Secure Enclave hardware. Decryption (ECDH) is performed
inside the SE; the key never leaves the hardware.

Uses CryptoTokenKit identities created via sc_auth, which allows
SE access from unsigned binaries without Apple Developer Program
membership. ECIES (X963SHA256 + AES-GCM) handles encryption and
decryption through Security.framework.

New package internal/macse/ provides the CGo bridge to
Security.framework for SE key creation, ECIES encrypt/decrypt,
and key deletion. The SE unlocker directly encrypts the vault
long-term key (no intermediate age keypair).
Fix nlreturn lint errors in macse CGo bindings e10b4cec82
sneak added 1 commit 2026-03-11 02:34:47 +01:00
Merge branch 'main' into secure-enclave-unlocker b9aaf8ddac
sneak requested review from clawbot 2026-03-11 02:35:13 +01:00
sneak added the needs-review label 2026-03-11 02:35:26 +01:00
clawbot requested changes 2026-03-11 02:40:36 +01:00
Dismissed
clawbot left a comment
Collaborator
Copy Link

Code Review: PR #24 — secure-enclave-unlocker

Verdict: ❌ FAIL

Policy Divergences

1. internal/secret/seunlocker_stub.go: panic() in library code

Every method in this file panics instead of returning an error. This violates AGENTS.md Go-Specific Guideline #1: "No panic, log.Fatal, or os.Exit in library code. Always propagate errors via return values."

The existing keychainunlocker_stub.go correctly returns errors for all methods and constructs a valid (but inert) struct from NewKeychainUnlocker. The SE stub should follow this exact same pattern. CreateSecureEnclaveUnlocker already has an error return — use it. The interface methods (GetIdentity, Remove) also have error returns. GetType, GetDirectory, GetID, GetMetadata can return sentinel values.

Files: internal/secret/seunlocker_stub.go lines 29-63, all methods.

2. internal/secret/seunlocker_darwin.go:280: Hardcoded derivation index 0

ltIdentity, err := agehd.DeriveIdentity(envMnemonic, 0)

getLongTermKeyForSE uses hardcoded derivation index 0 when deriving from the mnemonic env var. Compare with vault.go:GetOrDeriveLongTermKey() which correctly loads metadata.DerivationIndex from vault metadata. If the vault uses a derivation index other than 0, this will encrypt the wrong key into the SE unlocker, silently corrupting it. This is a correctness bug.

3. Missing tests for new exported types in secret package

The PR adds these new exports to internal/secret/ with zero test coverage:

  • SecureEnclaveUnlocker (type)
  • SecureEnclaveUnlockerMetadata (type)
  • NewSecureEnclaveUnlocker (constructor)
  • CreateSecureEnclaveUnlocker (function)

The macse package has good tests (macse_test.go), but the secret package SE unlocker code has none. The existing keychainunlocker_test.go demonstrates the expected pattern. At minimum, NewSecureEnclaveUnlocker should be tested (instantiation, getter methods) and CreateSecureEnclaveUnlocker should have a happy-path test (darwin-only, with SE availability skip guard).

4. README inconsistency

The README has not been updated to reflect this implementation:

  • Line 289: Still says "Secure Enclave Unlockers (macOS - planned)" with "Currently partially implemented but non-functional"
  • Line 333: Says "Secure Enclave support planned (requires paid Apple Developer Program for signed entitlements)"
  • Line 446: Says "planned Secure Enclave integration"
  • Lines 181-188: The unlocker add types list doesn't include secure-enclave
  • Lines 385-387: The "Advanced Authentication" examples don't include secret unlocker add secure-enclave

The implementation uses sc_auth / CryptoTokenKit which bypasses the entitlement requirement, so the README's claim about needing Apple Developer Program membership is now inaccurate.

Requirements Checklist

Requirement Status
Secure Enclave unlocker type ✅ Met
CGo bindings for SE operations (create/encrypt/decrypt/delete) ✅ Met
Non-darwin stubs for cross-platform compilation ⚠️ Compiles, but panics instead of returning errors
CLI integration (add, list, select) ✅ Met
Vault unlock flow integration ✅ Met
Tests for macse package ✅ Met (darwin-only)
Tests for secret package SE unlocker ❌ Missing
README updated ❌ Not updated
Correct mnemonic derivation index ❌ Hardcoded to 0

Build Result

docker build . — ✅ PASS (4.2s)

Additional Notes

  • The vault.go refactoring (extracting unlockLongTermKey) is clean and the type-dispatch approach for SE vs. standard unlockers is reasonable.
  • The macse CGo bindings and Objective-C implementation look well-structured with proper memory management (@autoreleasepool, CFRelease).
  • The macse_stub.go correctly returns errors (not panics) — this is the right pattern.
  • The hash parsing from sc_auth list-ctk-identities output (secure_enclave.m lines 161-179) is fragile — it depends on the output format of sc_auth. Consider a comment noting this.
  • No linter config, CI config, test assertion, or Makefile changes detected — no cheating.

Summary

Four issues require resolution: (1) stub panics must be replaced with error returns per existing codebase pattern, (2) derivation index bug must be fixed, (3) tests needed for secret package exports, (4) README must be updated.

## Code Review: PR #24 — secure-enclave-unlocker ### Verdict: ❌ FAIL --- ### Policy Divergences **1. `internal/secret/seunlocker_stub.go`: `panic()` in library code** Every method in this file panics instead of returning an error. This violates AGENTS.md Go-Specific Guideline #1: "No `panic`, `log.Fatal`, or `os.Exit` in library code. Always propagate errors via return values." The existing `keychainunlocker_stub.go` correctly returns errors for all methods and constructs a valid (but inert) struct from `NewKeychainUnlocker`. The SE stub should follow this exact same pattern. `CreateSecureEnclaveUnlocker` already has an error return — use it. The interface methods (`GetIdentity`, `Remove`) also have error returns. `GetType`, `GetDirectory`, `GetID`, `GetMetadata` can return sentinel values. Files: `internal/secret/seunlocker_stub.go` lines 29-63, all methods. **2. `internal/secret/seunlocker_darwin.go:280`: Hardcoded derivation index 0** ```go ltIdentity, err := agehd.DeriveIdentity(envMnemonic, 0) ``` `getLongTermKeyForSE` uses hardcoded derivation index `0` when deriving from the mnemonic env var. Compare with `vault.go:GetOrDeriveLongTermKey()` which correctly loads `metadata.DerivationIndex` from vault metadata. If the vault uses a derivation index other than 0, this will encrypt the **wrong key** into the SE unlocker, silently corrupting it. This is a correctness bug. **3. Missing tests for new exported types in `secret` package** The PR adds these new exports to `internal/secret/` with zero test coverage: - `SecureEnclaveUnlocker` (type) - `SecureEnclaveUnlockerMetadata` (type) - `NewSecureEnclaveUnlocker` (constructor) - `CreateSecureEnclaveUnlocker` (function) The `macse` package has good tests (`macse_test.go`), but the `secret` package SE unlocker code has none. The existing `keychainunlocker_test.go` demonstrates the expected pattern. At minimum, `NewSecureEnclaveUnlocker` should be tested (instantiation, getter methods) and `CreateSecureEnclaveUnlocker` should have a happy-path test (darwin-only, with SE availability skip guard). **4. README inconsistency** The README has not been updated to reflect this implementation: - Line 289: Still says "Secure Enclave Unlockers (macOS - planned)" with "Currently partially implemented but non-functional" - Line 333: Says "Secure Enclave support planned (requires paid Apple Developer Program for signed entitlements)" - Line 446: Says "planned Secure Enclave integration" - Lines 181-188: The `unlocker add` types list doesn't include `secure-enclave` - Lines 385-387: The "Advanced Authentication" examples don't include `secret unlocker add secure-enclave` The implementation uses `sc_auth` / CryptoTokenKit which bypasses the entitlement requirement, so the README's claim about needing Apple Developer Program membership is now inaccurate. --- ### Requirements Checklist | Requirement | Status | |---|---| | Secure Enclave unlocker type | ✅ Met | | CGo bindings for SE operations (create/encrypt/decrypt/delete) | ✅ Met | | Non-darwin stubs for cross-platform compilation | ⚠️ Compiles, but panics instead of returning errors | | CLI integration (add, list, select) | ✅ Met | | Vault unlock flow integration | ✅ Met | | Tests for `macse` package | ✅ Met (darwin-only) | | Tests for `secret` package SE unlocker | ❌ Missing | | README updated | ❌ Not updated | | Correct mnemonic derivation index | ❌ Hardcoded to 0 | --- ### Build Result `docker build .` — ✅ PASS (4.2s) --- ### Additional Notes - The `vault.go` refactoring (extracting `unlockLongTermKey`) is clean and the type-dispatch approach for SE vs. standard unlockers is reasonable. - The `macse` CGo bindings and Objective-C implementation look well-structured with proper memory management (`@autoreleasepool`, `CFRelease`). - The `macse_stub.go` correctly returns errors (not panics) — this is the right pattern. - The hash parsing from `sc_auth list-ctk-identities` output (secure_enclave.m lines 161-179) is fragile — it depends on the output format of `sc_auth`. Consider a comment noting this. - No linter config, CI config, test assertion, or Makefile changes detected — no cheating. ### Summary Four issues require resolution: (1) stub panics must be replaced with error returns per existing codebase pattern, (2) derivation index bug must be fixed, (3) tests needed for `secret` package exports, (4) README must be updated.
sneak commented 2026-03-11 14:31:08 +01:00
Author
Owner
Copy Link

@clawbot pls rework

@clawbot pls rework
clawbot added needs-rework and removed needs-review labels 2026-03-11 14:31:54 +01:00
clawbot commented 2026-03-11 14:32:01 +01:00
Collaborator
Copy Link

Rework agent dispatched to address review findings: (1) stub panics → error returns, (2) hardcoded derivation index fix, (3) missing tests for secret package SE exports, (4) README update.

Rework agent dispatched to address review findings: (1) stub panics → error returns, (2) hardcoded derivation index fix, (3) missing tests for secret package SE exports, (4) README update.
clawbot force-pushed secure-enclave-unlocker from b9aaf8ddac to cc53469f90 2026-03-11 14:36:47 +01:00 Compare
clawbot commented 2026-03-11 14:37:08 +01:00
Collaborator
Copy Link

Rework Complete

All four review findings have been addressed:

1. Stub panics → error returns

internal/secret/seunlocker_stub.go now follows the keychainunlocker_stub.go pattern exactly:

  • Sentinel error errSENotSupported used for GetIdentity(), Remove(), and CreateSecureEnclaveUnlocker()
  • GetType() returns "secure-enclave", GetMetadata() returns stored metadata, GetDirectory() returns stored directory, GetID() returns formatted string
  • NewSecureEnclaveUnlocker() returns a valid (inert) struct instance

2. Hardcoded derivation index 0 → vault metadata

getLongTermKeyForSE() now reads vault-metadata.json to get metadata.DerivationIndex, matching the pattern in getLongTermPrivateKey() from keychainunlocker.go. This prevents silent key corruption when vaults use non-zero derivation indices.

3. Tests for secret package SE exports

Added two test files:

  • seunlocker_stub_test.go (non-darwin): tests NewSecureEnclaveUnlocker instantiation/getters, error returns from GetIdentity/Remove/CreateSecureEnclaveUnlocker, and interface compliance
  • seunlocker_test.go (darwin): tests instantiation, interface compliance, GetID format, generateSEKeyLabel, and GetIdentity with missing encrypted key file

4. README updated

  • Removed all "planned" / "partially implemented" / "non-functional" language
  • Removed incorrect Apple Developer Program requirement (implementation uses sc_auth / CryptoTokenKit)
  • Added secure-enclave to unlocker type list in CLI reference
  • Added secret unlocker add secure-enclave to Advanced Authentication examples
  • Updated Cross-Platform Support and Features sections

docker build . ✅ PASS

## Rework Complete All four review findings have been addressed: ### 1. Stub panics → error returns `internal/secret/seunlocker_stub.go` now follows the `keychainunlocker_stub.go` pattern exactly: - Sentinel error `errSENotSupported` used for `GetIdentity()`, `Remove()`, and `CreateSecureEnclaveUnlocker()` - `GetType()` returns `"secure-enclave"`, `GetMetadata()` returns stored metadata, `GetDirectory()` returns stored directory, `GetID()` returns formatted string - `NewSecureEnclaveUnlocker()` returns a valid (inert) struct instance ### 2. Hardcoded derivation index 0 → vault metadata `getLongTermKeyForSE()` now reads `vault-metadata.json` to get `metadata.DerivationIndex`, matching the pattern in `getLongTermPrivateKey()` from `keychainunlocker.go`. This prevents silent key corruption when vaults use non-zero derivation indices. ### 3. Tests for `secret` package SE exports Added two test files: - `seunlocker_stub_test.go` (non-darwin): tests `NewSecureEnclaveUnlocker` instantiation/getters, error returns from `GetIdentity`/`Remove`/`CreateSecureEnclaveUnlocker`, and interface compliance - `seunlocker_test.go` (darwin): tests instantiation, interface compliance, `GetID` format, `generateSEKeyLabel`, and `GetIdentity` with missing encrypted key file ### 4. README updated - Removed all "planned" / "partially implemented" / "non-functional" language - Removed incorrect Apple Developer Program requirement (implementation uses `sc_auth` / CryptoTokenKit) - Added `secure-enclave` to unlocker type list in CLI reference - Added `secret unlocker add secure-enclave` to Advanced Authentication examples - Updated Cross-Platform Support and Features sections `docker build .` ✅ PASS
clawbot added needs-review and removed needs-rework labels 2026-03-12 03:01:18 +01:00
clawbot approved these changes 2026-03-12 03:42:13 +01:00
clawbot left a comment
Collaborator
Copy Link

Code Review: PR #24 — secure-enclave-unlocker (post-rework)

Verdict: ✅ PASS

Previous Review Findings — Resolution Status

All four findings from the initial review have been properly addressed:

Finding Status Verification
1. Stub panics → error returns ✅ Fixed seunlocker_stub.go now uses sentinel errSENotSupported, matching keychainunlocker_stub.go exactly
2. Hardcoded derivation index 0 ✅ Fixed getLongTermKeyForSE() reads vault-metadata.json for metadata.DerivationIndex, matching keychainunlocker.go pattern
3. Missing tests for secret package SE exports ✅ Fixed Two test files: seunlocker_stub_test.go (non-darwin) and seunlocker_test.go (darwin), covering constructor, getters, interface compliance, error paths
4. README inconsistency ✅ Fixed All "planned"/"partially implemented" language removed, SE added to unlocker type list, examples, features, and cross-platform sections

Policy Divergences

No policy violations found in the changed files.

  • No panic, log.Fatal, or os.Exit in library code ✅
  • Errors wrapped with fmt.Errorf("context: %w", err) throughout ✅
  • No changes to .golangci.yml, Makefile, or CI config ✅
  • No changes to go.mod/go.sum (no new dependencies) ✅
  • No weakened tests or suppressed lint rules ✅
  • Constants used for repeated strings (seKeyLabelPrefix, seUnlockerType, seLongtermFilename, p256UncompressedKeySize, etc.) ✅

Requirements Checklist

Requirement Status
Secure Enclave unlocker type ✅ Met
CGo bindings for SE operations (create/encrypt/decrypt/delete) ✅ Met
Non-darwin stubs — compile and return errors ✅ Met
CLI integration (add, list, select) ✅ Met
Vault unlock flow integration (unlockLongTermKey refactor) ✅ Met
Tests for macse package (darwin) ✅ Met
Tests for secret package SE unlocker (both platforms) ✅ Met
README updated to reflect implementation ✅ Met
Correct mnemonic derivation index from vault metadata ✅ Met

Build Result

docker build . — ✅ PASS (4.3s)

Rebase onto main — ✅ Clean (already up to date)

Code Quality Notes

  • vault.go refactoring (unlockLongTermKey extraction) is clean and the type-dispatch for SE vs. standard unlockers is architecturally sound.
  • Memory handling is good: memguard.LockedBuffer used for key material, decryptedData zeroed immediately after parsing in GetIdentity().
  • Objective-C implementation has proper @autoreleasepool blocks and CFRelease calls — no memory leaks.
  • Stub GetID() omits hostname (returns timestamp-secure-enclave vs darwin's timestamp-hostname-secure-enclave) — this matches the existing KeychainUnlocker stub pattern exactly.
  • The sc_auth list-ctk-identities output parsing in secure_enclave.m is fragile (depends on whitespace-tokenized format) — previously noted, not blocking.
## Code Review: [PR #24](https://git.eeqj.de/sneak/secret/pulls/24) — secure-enclave-unlocker (post-rework) ### Verdict: ✅ PASS --- ### Previous Review Findings — Resolution Status All four findings from the [initial review](https://git.eeqj.de/sneak/secret/pulls/24#issuecomment-13251) have been properly addressed: | Finding | Status | Verification | |---|---|---| | 1. Stub panics → error returns | ✅ Fixed | `seunlocker_stub.go` now uses sentinel `errSENotSupported`, matching `keychainunlocker_stub.go` exactly | | 2. Hardcoded derivation index 0 | ✅ Fixed | `getLongTermKeyForSE()` reads `vault-metadata.json` for `metadata.DerivationIndex`, matching `keychainunlocker.go` pattern | | 3. Missing tests for `secret` package SE exports | ✅ Fixed | Two test files: `seunlocker_stub_test.go` (non-darwin) and `seunlocker_test.go` (darwin), covering constructor, getters, interface compliance, error paths | | 4. README inconsistency | ✅ Fixed | All "planned"/"partially implemented" language removed, SE added to unlocker type list, examples, features, and cross-platform sections | --- ### Policy Divergences No policy violations found in the changed files. - No `panic`, `log.Fatal`, or `os.Exit` in library code ✅ - Errors wrapped with `fmt.Errorf("context: %w", err)` throughout ✅ - No changes to `.golangci.yml`, `Makefile`, or CI config ✅ - No changes to `go.mod`/`go.sum` (no new dependencies) ✅ - No weakened tests or suppressed lint rules ✅ - Constants used for repeated strings (`seKeyLabelPrefix`, `seUnlockerType`, `seLongtermFilename`, `p256UncompressedKeySize`, etc.) ✅ --- ### Requirements Checklist | Requirement | Status | |---|---| | Secure Enclave unlocker type | ✅ Met | | CGo bindings for SE operations (create/encrypt/decrypt/delete) | ✅ Met | | Non-darwin stubs — compile and return errors | ✅ Met | | CLI integration (add, list, select) | ✅ Met | | Vault unlock flow integration (`unlockLongTermKey` refactor) | ✅ Met | | Tests for `macse` package (darwin) | ✅ Met | | Tests for `secret` package SE unlocker (both platforms) | ✅ Met | | README updated to reflect implementation | ✅ Met | | Correct mnemonic derivation index from vault metadata | ✅ Met | --- ### Build Result `docker build .` — ✅ PASS (4.3s) Rebase onto `main` — ✅ Clean (already up to date) --- ### Code Quality Notes - `vault.go` refactoring (`unlockLongTermKey` extraction) is clean and the type-dispatch for SE vs. standard unlockers is architecturally sound. - Memory handling is good: `memguard.LockedBuffer` used for key material, `decryptedData` zeroed immediately after parsing in `GetIdentity()`. - Objective-C implementation has proper `@autoreleasepool` blocks and `CFRelease` calls — no memory leaks. - Stub `GetID()` omits hostname (returns `timestamp-secure-enclave` vs darwin's `timestamp-hostname-secure-enclave`) — this matches the existing `KeychainUnlocker` stub pattern exactly. - The `sc_auth list-ctk-identities` output parsing in `secure_enclave.m` is fragile (depends on whitespace-tokenized format) — previously noted, not blocking.
clawbot added merge-ready and removed needs-review labels 2026-03-12 23:09:21 +01:00
sneak was assigned by clawbot 2026-03-12 23:09:36 +01:00
sneak added 4 commits 2026-03-13 13:56:13 +01:00
Add Secure Enclave unlocker for hardware-backed secret protection 4adeeae1db 
Adds a new "secure-enclave" unlocker type that stores the vault's
long-term private key encrypted by a non-exportable P-256 key held
in the Secure Enclave hardware. Decryption (ECDH) is performed
inside the SE; the key never leaves the hardware.

Uses CryptoTokenKit identities created via sc_auth, which allows
SE access from unsigned binaries without Apple Developer Program
membership. ECIES (X963SHA256 + AES-GCM) handles encryption and
decryption through Security.framework.

New package internal/macse/ provides the CGo bridge to
Security.framework for SE key creation, ECIES encrypt/decrypt,
and key deletion. The SE unlocker directly encrypts the vault
long-term key (no intermediate age keypair).
Fix nlreturn lint errors in macse CGo bindings e10b4cec82
Merge branch 'main' into secure-enclave-unlocker b9aaf8ddac
Merge branch 'secure-enclave-unlocker' of git.eeqj.de:sneak/secret into secure-enclave-unlocker 02f253fc02
sneak merged commit a3d3fb3b69 into main 2026-03-14 07:36:28 +01:00
sneak deleted branch secure-enclave-unlocker 2026-03-14 07:36:29 +01:00
sneak referenced this issue from a commit 2026-03-14 07:36:29 +01:00
secure-enclave-unlocker (#24)
Sign in to join this conversation.
Reviewers
No Reviewers
clawbot
Labels
Clear labels
merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No Label merge-ready
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees sneak
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/secret#24
Delete Branch "secure-enclave-unlocker"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?