feat(iOS): keychain mnemonic security

[alienx5499]

Summary

Secures the BIP-39 wallet mnemonic on iOS by migrating it from legacy plaintext disk storage (seed_phrase) to Apple's hardware-secured Keychain.

The Keychain entry is configured with kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly within the group.com.stablechannels.app shared App Group domain. This allows the background NotificationService extension to wake up, boot the LDK Node, and process inbound stability payments when the main app is closed and the device is locked, while preventing cleartext exposure or iCloud syncing.

---

Problem & Threat Model

1. Cleartext Seed Exposure: Storing the seed phrase in a plaintext file on disk leaves it exposed to anyone with filesystem access or unencrypted device backups.
2. Background Wakeups (NSE): The background Notification Service Extension needs to read the seed to boot the LDK Node on push notifications. Gating the Keychain under direct biometric access controls (kSecAccessControl / Face ID prompt) would block silent background receive operations.
3. Destructive Mutation Risks: Blindly rewriting seeds or wiping state during restore/migration without transactional safety risks permanent funds loss on unexpected process termination (OOM/jetsam).

---

Key Solutions & Architecture

1. WalletKeychainService (Encrypted-First & Non-Destructive):

   • Implements MnemonicStorageProtocol with kSecClassGenericPassword and kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly.
   • Idempotent, non-destructive writes: Checks for same-value no-op, uses SecItemUpdate when existing, SecItemAdd when absent, followed by read-after-write verification (never delete-then-add).
   • Dedicated pending slot (seed_phrase_pending) for atomic staged restore transactions.

2. MnemonicMigrator (Fail-Closed & Encrypted-First):

   • Keychain is strictly authoritative: if Keychain already holds a seed, legacy plaintext is deleted.
   • If both Keychain and plaintext coexist with differing phrases, logs KEYCHAIN_PLAINTEXT_MISMATCH and preserves the Keychain seed without overwriting.
   • Plaintext migration is only permitted on WalletKeychainError.keyNotFound. Any operational error fails closed.

3. WalletLifecycleManager (Staged Restore Transaction & State Machine):

   • Pre-flight BIP-39 Validation: Validates words and checksum before any storage mutation or node stop.
   • Durable Two-Phase Commit: Stages new seed in pending slot (.pendingValidation), stops node, wipes old database throwing (.oldPersistenceWiped), promotes new seed to active slot, cleans pending slot, and clears phase.
   • Automatic Crash Recovery: If interrupted mid-restore, startup recovers state safely based on the durable phase marker in UserDefaults.
   • Startup Mismatch Matrix: Distinguishes .ready, .newWallet, .seedOnlyMismatch, .dbOnlyMismatch, .seedStorageMismatch, and .storageError.

4. Safe Seed Recovery UX:

   • Mismatched startup states present the interactive RestoreSeedSheet, requiring the user to supply and validate their backup phrase before any state is modified.

---

Architecture Diagram

---

Verification & Testing

• All Unit & Integration Tests Passing: 201 tests passing (xcodebuild test).
• Isolation: Tests use unique isolated namespaces / mocks, preventing pollution of developer device Keychains.
• Zero Warnings / SwiftFormat Clean: Codebase conforms strictly to Swift concurrency, SOLID principles, and repository formatting standards.

---

Related

• Closes iOS: Hardware-Secured Wallet Seed Storage and Background Migration #236

[toneloc]

Thanks, can you please add some tests and have you tested this on-device?

[toneloc]

Please check the below:

Comment 1 — [P1] Delete-before-rewrite can permanently lose the only seed

storeMnemonic() writes with SecItemDelete immediately followed by SecItemAdd, and start() calls storeMnemonic(words) unconditionally on every existing-wallet startup — including when words was just loaded from the Keychain. After the first migration the plaintext seed_phrase file is gone, so on every subsequent boot the Keychain is the sole copy of the seed, and it is transiently deleted on each launch.

Two ways this loses the seed for good in the gap:

• The process is killed between the delete and the add (iOS jetsam/OOM, or the NSE's tight time/memory budget) → permanent loss.
• SecItemAdd fails after the delete succeeded → storeMnemonic throws, but the item is already gone. The read-after-write verification runs after the add, so it can't catch this case.

Consequence: unrecoverable funds loss unless the user still has their written backup. This is a routine code path, not an edge case.

Fix:

1. Make the write non-destructive: SecItemUpdate when the item exists, SecItemAdd only when it's missing — never delete-then-add.
2. Additionally, don't rewrite at all when the Keychain already holds the same seed, so the common startup path performs zero destructive writes.

func storeMnemonic(_ mnemonic: String) throws {
    let trimmed = mnemonic.trimmingCharacters(in: .whitespacesAndNewlines)
    guard !trimmed.isEmpty, let data = trimmed.data(using: .utf8) else {
        throw WalletKeychainError.dataConversionFailed
    }

    let base: [String: Any] = [
        kSecClass as String: kSecClassGenericPassword,
        kSecAttrService as String: service,
        kSecAttrAccount as String: account,
        kSecAttrAccessGroup as String: accessGroup
    ]

    // No-op if the stored value already matches — avoids any unnecessary write.
    if let existing = try? loadMnemonic(), existing == trimmed { return }

    if hasMnemonic() {
        let attrs: [String: Any] = [
            kSecValueData as String: data,
            kSecAttrAccessible as String: kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly
        ]
        let status = SecItemUpdate(base as CFDictionary, attrs as CFDictionary)
        guard status == errSecSuccess else {
            logError("KEYCHAIN_STORE_FAILED", data: ["op": "update", "status": String(status)])
            throw WalletKeychainError.accessDenied(status)
        }
    } else {
        var attrs = base
        attrs[kSecValueData as String] = data
        attrs[kSecAttrAccessible as String] = kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly
        let status = SecItemAdd(attrs as CFDictionary, nil)
        guard status == errSecSuccess else {
            logError("KEYCHAIN_STORE_FAILED", data: ["op": "add", "status": String(status)])
            throw WalletKeychainError.accessDenied(status)
        }
    }

    // Read-after-write verification.
    guard (try? loadMnemonic()) == trimmed else {
        logError("KEYCHAIN_VERIFICATION_FAILED", data: [:])
        throw WalletKeychainError.dataConversionFailed
    }
}

Note init()'s migration is less dangerous than start()'s rewrite, because the plaintext file still exists during the gap as a fallback — the uniquely fatal case is start() re-storing after the plaintext has already been deleted.

---

Comment 2 — [P1] Plaintext overrides an existing Keychain wallet (encrypted-first violated)

init() checks the plaintext seed_phrase file first and, if present, stores it into the Keychain without checking whether the Keychain already holds a different seed and without comparing values, then deletes the plaintext with try? (error ignored). That inverts the encrypted-first rule from #229: plaintext wins over the protected store.

If the two ever coexist and differ — e.g. a prior migration whose plaintext delete silently failed and left the file behind, or a seed_phrase resurrected from a device backup while the Keychain holds the current wallet — the stale/planted plaintext becomes authoritative and clobbers the real Keychain wallet.

Fix: read the Keychain first; migrate the plaintext only when the Keychain is empty; if both exist, compare and treat a mismatch as an error to surface rather than a silent overwrite; and stop ignoring the plaintext-delete failure.

init() {
    let path = Constants.userDataDir.appendingPathComponent("seed_phrase")

    // Encrypted-first: an existing Keychain seed is authoritative.
    if let keychainMnemonic = try? WalletKeychainService.shared.loadMnemonic() {
        savedMnemonic = keychainMnemonic

        // If a plaintext file lingers, reconcile rather than blindly overwrite.
        if let plaintext = try? String(contentsOfFile: path.path, encoding: .utf8) {
            let trimmed = plaintext.trimmingCharacters(in: .whitespacesAndNewlines)
            if !trimmed.isEmpty, trimmed != keychainMnemonic {
                // Two different seeds present — do NOT overwrite. Surface it.
                AuditService.log("KEYCHAIN_PLAINTEXT_MISMATCH", data: [:])
                return
            }
            // Same value (or empty): safe to remove the plaintext, and report if it fails.
            do { try FileManager.default.removeItem(at: path) }
            catch { AuditService.log("KEYCHAIN_PLAINTEXT_DELETE_FAILED",
                                     data: ["error": error.localizedDescription]) }
        }
        return
    }

    // Keychain empty — migrate plaintext if present.
    guard let words = try? String(contentsOfFile: path.path, encoding: .utf8) else { return }
    let trimmed = words.trimmingCharacters(in: .whitespacesAndNewlines)
    guard !trimmed.isEmpty else { return }
    savedMnemonic = trimmed
    do {
        try WalletKeychainService.shared.storeMnemonic(trimmed) // verified write
        do { try FileManager.default.removeItem(at: path) }
        catch { AuditService.log("KEYCHAIN_PLAINTEXT_DELETE_FAILED",
                                 data: ["error": error.localizedDescription]) }
    } catch {
        // Migration failed — keep the plaintext so nothing is lost, and log it.
        AuditService.log("KEYCHAIN_MIGRATION_FAILED", data: ["error": error.localizedDescription])
    }
}

Combined with Comment 1, the theme is: the write path must be idempotent and non-destructive, and the read path must be strictly encrypted-first.

[alienx5499]

Thanks for the thorough review @toneloc

Both points are addressed in the latest update:

1. Non-destructive Keychain writes: Updated storeMnemonic() to early-return if the stored value matches, and use SecItemUpdate / SecItemAdd instead of SecItemDelete + SecItemAdd.
2. Encrypted-first initialization: Re-ordered NodeService.init() to check the Keychain first as authoritative, safely reconciling lingering plaintext files without overwriting existing seeds.
3. Unit Tests: Added WalletKeychainServiceTests covering round-trips, idempotency, updates, whitespace trimming, and deletion.

Tested and verified on my devices.

[toneloc]

Comment 1 — [P1] Restore can leave an old seed paired with an empty channel database

NodeService.wipeWalletData() deletes the LDK database files before calling WalletKeychainService.deleteMnemonic(). However, deleteMnemonic() only logs Keychain deletion failures and does not throw or report failure.

During restoreWalletFromMnemonic(), this can produce the following sequence:

1. The existing LDK channel database is deleted.
2. Deleting the old Keychain seed fails.
3. Storing the replacement seed also fails.
4. Restore exits with an error, leaving the old seed but no channel database.
5. On the next launch, hasMnemonic() classifies this as an existing wallet and starts the old node identity with empty LDK state.

The restore code already documents that starting a node identity without its existing channel state can cause the counterparty to force-close the channel.

Fix:

• Make deleteMnemonic() throw or return its OSStatus.
• Do not delete the LDK database if the seed transition cannot proceed.
• Make startup explicitly distinguish these states:
  • seed and database both present;
  • seed present but database missing;
  • database present but seed missing;
  • neither present.
• Never automatically start the node in either mismatched state.
• Prefer a staged restore transaction: store and verify a pending seed, record a restore marker, wipe the old state, activate the new seed, then clear the marker.

The same state handling is needed for device migration because kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly items do not migrate to a new device.

---

Comment 2 — [P1] Keychain tests operate on the production wallet item

WalletKeychainServiceTests and MnemonicMigratorTests use WalletKeychainService.shared, including calls to deleteMnemonic() and storeMnemonic().

The “test-specific mnemonic” does not isolate these tests. Keychain items are identified by their service, account, and access group—not by the mnemonic value. The tests therefore access the same item used by the actual wallet:

• service: com.stablechannels.wallet
• account: seed_phrase
• access group: group.com.stablechannels.app

Running the tests on a development device can delete the real wallet seed, replace it with a test mnemonic, or leave a test mnemonic behind after the suite finishes.

Fix:

• Make WalletKeychainService constructible with configurable service and account values, or inject a Keychain storage protocol.
• Give every test a unique service/account namespace.
• Never use WalletKeychainService.shared in tests that store or delete data.
• Add teardown that removes only the test-specific item.

---

Comment 3 - [P2] Migration treats every Keychain read error as “not found”

MnemonicMigrator.loadOrMigrateMnemonic() currently uses:

if let keychainMnemonic = try? keychain.loadMnemonic()

This treats access denial, unexpected Security errors, and invalid stored data the same as a missing item. The migrator can then fall back to plaintext even though a Keychain item may exist.

Only WalletKeychainError.keyNotFound should authorize plaintext migration. Other errors should fail closed and leave both stores untouched.

Fix:

do {
    let keychainMnemonic = try keychain.loadMnemonic()
    // Keychain is authoritative; reconcile any plaintext file.
    return keychainMnemonic
} catch WalletKeychainError.keyNotFound {
    // Keychain is genuinely empty; plaintext migration is allowed.
} catch {
    logError?("KEYCHAIN_LOAD_FAILED", [
        "error": error.localizedDescription
    ])
    return nil
}

Add tests confirming that:

• keyNotFound permits plaintext migration.
• accessDenied does not trigger plaintext migration.
• Invalid Keychain data does not silently fall back.
• A Keychain/plaintext mismatch keeps the Keychain value authoritative.

[alienx5499]

Thanks @toneloc! Addressed all points: implemented throwing Keychain deletion with a staged two-phase restore transaction (WalletLifecycleManager), isolated tests to unique namespaces/mocks, and made migration fail closed on all Keychain errors except keyNotFound.

[toneloc]

Thanks. This is again much improved. It resolves many of the prior concerns.

I am giving this PR a lot of scrutiny because it handles keys. Please assess the below comments for accuracy:

I found two remaining lifecycle issues. 1. should be fixed, 2. is not as important.

1. Blocker: the restore wipe deletes the pending recovery seed.

   WalletLifecycleManager stores the replacement seed in the pending Keychain slot before wiping, but AppState.wipeAllWalletState() deletes both the active and pending slots.

   Normal restoration succeeds because the seed remains in memory. However, if the process dies after the wipe but before promotion, both durable copies are gone. On restart, recovery sees no pending seed, clears the restore marker, and may proceed as a new wallet.

   The persistence wipe should never delete the pending seed. Only WalletLifecycleManager should remove it after verified promotion. If a restore marker exists but neither the pending nor active seed exists, recovery should fail closed instead of clearing the marker. Please also add an integration test using the real wipe behavior—the current mocks delete only the active seed, so they do not reproduce this failure.

2. Important: the notification extension bypasses the new mismatch protection.

   The main app blocks seed-without-database and other mismatched states, but the notification extension checks only whether a seed exists. After the main app detects a missing database and releases its lock, a push can start the extension and create a fresh LDK database from that seed, defeating the guard and potentially triggering the force-close scenario it is intended to prevent.

   The extension should run the same lightweight startup preflight: refuse to start while a restore is in progress or when the seed/database state is mismatched, and defer processing through pending_push_payment.

The focused Keychain, migration, and lifecycle tests pass, as does git diff --check.

[alienx5499]

Thanks @toneloc!
Fixed both: restore wipe now preserves the staged pending seed (recovery fails closed if missing), and the notification extension runs preflight checks to refuse startup on restore or seed/DB mismatch.

[toneloc]

Still working on this and assessing. Very mission-critical code we are touching here @alienx5499

[toneloc]

I am still reviewing this. I run this past different AI models, and it keeps finding different bugs! Some of which I have tried to fix here.

So I will keep this open until this approach further matured and/or we get more comfortable with it.

[toneloc]

Pushed 703b45b closing the two remaining findings from the reconciled review (see pr-237-final.md in the repo tooling; both reviewers converged on these):

• Implicit wallet creation is gone, with two independent gates. start() gained allowCreate: Bool (threaded through NodeServiceProtocol, startNodeWithFailover, and both LSPService call sites) — only the lifecycle-confirmed .newWallet branch passes true. Independently of the flag, generation now refuses unconditionally while ldk_node_data.sqlite exists: seedless-with-db is .dbOnlyMismatch, and wiping monitors to mint a fresh identity is the historic force-close class. Foreground restart, LSP switch, and the Esplora failover retry throw the new walletStateMismatch error instead. The second gate also protects any future mis-gated call site.
• The migrator's one fail-open is closed. A seed_phrase that exists but cannot be read (permissions, I/O error, partial write) no longer reads as "absent" — it throws the new plaintextUnreadable instead of returning the nil that routed into wipe-and-generate. Covered by a permissions-based test (testUnreadablePlaintextThrowsInsteadOfReadingAsAbsent).

Verified on the branch: 267/267 tests pass in the simulator (full scheme incl. the NSE), swiftformat --lint --strict clean.

With these two in, the review verdict is approve. Remaining follow-ups are non-blocking and filed in the review: node-ID cross-check as defense-in-depth, and the phase-2 gate list (backup-restore warning copy must ship BEFORE plaintext deletion). A final merge of main before landing is recommended — the branch is ~10 commits behind and merges cleanly.

[alienx5499]

Thanks for the thorough review and closing out those final findings @toneloc!

I have rebased the branch on top of latest main with all changes and tests cleanly reconciled.

All 267 tests are passing and SwiftFormat is clean.