From afb1356b2711b7ef33ec22a0650d1398946404f0 Mon Sep 17 00:00:00 2001 From: Alan Wizemann Date: Mon, 27 Apr 2026 13:53:06 +0200 Subject: [PATCH] feat(ios-keychain): opt-in iCloud Keychain sync for SSH keys (#52) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reddit-reported friction: every iOS device needed its own SSH key because Scarf hardcoded kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly + kSecAttrSynchronizable=false on every Keychain write. Pairing iPhone + iPad meant onboarding twice and editing authorized_keys per device. Add an opt-in toggle in System tab → Security: - New SSHKeyICloudPreference (UserDefaults wrapper, default false so existing installs see no change on update). - KeychainSSHKeyStore.writeBundle now consults the preference: when on, items use kSecAttrAccessibleAfterFirstUnlock (no ThisDeviceOnly suffix — required for iCloud Keychain sync) + kSecAttrSynchronizable=true. - All read / list / delete queries unconditionally pass kSecAttrSynchronizable=kSecAttrSynchronizableAny so they match items regardless of sync state. Without this a flipped write would orphan items at the next read. - Public migrateAllItems(toICloudSync:) reads every stored bundle, deletes with Any, re-saves with target attributes. Idempotent. System tab Security section toggle: - Live migration on flip with a "Updating Keychain..." progress row. - Failure path reverts the toggle + surfaces the error inline rather than silently leaving the state inconsistent. - Footer copy explains the tradeoff (E2EE via iCloud Keychain; Advanced Data Protection keeps encryption keys on device). Out of scope: per-server-key sync override (M9 multi-server keys all sync or none); in-app key export. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../ScarfIOS/KeychainSSHKeyStore.swift | 143 ++++++++++++++---- .../ScarfIOS/SSHKeyICloudPreference.swift | 39 +++++ scarf/Scarf iOS/App/ScarfGoTabRoot.swift | 68 +++++++++ 3 files changed, 222 insertions(+), 28 deletions(-) create mode 100644 scarf/Packages/ScarfIOS/Sources/ScarfIOS/SSHKeyICloudPreference.swift diff --git a/scarf/Packages/ScarfIOS/Sources/ScarfIOS/KeychainSSHKeyStore.swift b/scarf/Packages/ScarfIOS/Sources/ScarfIOS/KeychainSSHKeyStore.swift index c1df30b..da18bce 100644 --- a/scarf/Packages/ScarfIOS/Sources/ScarfIOS/KeychainSSHKeyStore.swift +++ b/scarf/Packages/ScarfIOS/Sources/ScarfIOS/KeychainSSHKeyStore.swift @@ -17,9 +17,18 @@ import ScarfCore /// go here; v1 item is migrated into v2 on first `listAll()` after /// the upgrade, then removed. /// -/// All items use `kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly` -/// so they're reachable after a single device unlock (background -/// tasks, notification actions) but never sync to iCloud Keychain. +/// **Accessibility / sync attributes.** Default behavior pins items +/// to this device with `kSecAttrAccessibleAfterFirstUnlockThisDevice +/// Only` + `kSecAttrSynchronizable=false`. Users can opt into iCloud +/// Keychain sync via `SSHKeyICloudPreference` (issue #52); when +/// enabled, writes use `kSecAttrAccessibleAfterFirstUnlock` (no +/// `ThisDeviceOnly` suffix) + `kSecAttrSynchronizable=true` so the +/// key is picked up by iCloud Keychain on every signed-in device. +/// +/// All read / list / delete queries pass `kSecAttrSynchronizable = +/// kSecAttrSynchronizableAny` so they match items regardless of +/// sync state — load-bearing during the migration window when +/// device-only and synced items can briefly coexist. public struct KeychainSSHKeyStore: SSHKeyStore { public static let defaultService = "com.scarf.ssh-key" public static let legacyV1Account = "primary" @@ -56,10 +65,12 @@ public struct KeychainSSHKeyStore: SSHKeyStore { public func delete() async throws { // Wipe every v2 entry + the legacy v1 entry. Single-query delete - // that matches any account under our service. + // that matches any account under our service. Pass `Any` so the + // wipe catches synced + device-only items uniformly (issue #52). let query: [String: Any] = [ - kSecClass as String: kSecClassGenericPassword, - kSecAttrService as String: service, + kSecClass as String: kSecClassGenericPassword, + kSecAttrService as String: service, + kSecAttrSynchronizable as String: kSecAttrSynchronizableAny, ] let status = SecItemDelete(query as CFDictionary) if status != errSecSuccess && status != errSecItemNotFound { @@ -74,10 +85,13 @@ public struct KeychainSSHKeyStore: SSHKeyStore { public func listAll() async throws -> [ServerID] { migrateLegacyIfNeeded() let query: [String: Any] = [ - kSecClass as String: kSecClassGenericPassword, - kSecAttrService as String: service, - kSecReturnAttributes as String: true, - kSecMatchLimit as String: kSecMatchLimitAll, + kSecClass as String: kSecClassGenericPassword, + kSecAttrService as String: service, + kSecReturnAttributes as String: true, + kSecMatchLimit as String: kSecMatchLimitAll, + // Match items regardless of sync state (issue #52). Without + // this the listing silently misses synced items. + kSecAttrSynchronizable as String: kSecAttrSynchronizableAny, ] var items: CFTypeRef? let status = SecItemCopyMatching(query as CFDictionary, &items) @@ -115,15 +129,60 @@ public struct KeychainSSHKeyStore: SSHKeyStore { try deleteBundle(account: Self.multiAccountPrefix + id.uuidString) } + // MARK: - iCloud sync migration (issue #52) + + /// Migrate every stored key bundle to the requested sync state and + /// persist the user's preference for future writes. + /// + /// Idempotent: if the user enables sync twice in a row the second + /// call simply re-saves with the same attributes. Safe to call + /// from a UI toggle handler. Errors thrown by individual key + /// re-writes propagate; partial migrations are tolerable because + /// the read paths use `kSecAttrSynchronizableAny` and pick up + /// either copy on the next read. + /// + /// Side effects: + /// - Each stored key is read with `Any`, deleted with `Any`, then + /// re-saved with the target sync attributes via `writeBundle(_:account:syncToICloud:)`. + /// - The legacy v1 entry (if present) is migrated to the v2 layout + /// with the new attributes in passing. + /// - `SSHKeyICloudPreference.isEnabled` is set BEFORE the rewrite + /// loop so any concurrent `save(_:)` call from another path + /// already uses the right attributes. + public func migrateAllItems(toICloudSync enabled: Bool) async throws { + SSHKeyICloudPreference.isEnabled = enabled + + // Pull every v2 + v1 bundle into memory first. We can't iterate + // and rewrite simultaneously: deleting an item we're about to + // re-add would race with the listing query. + var bundles: [(account: String, bundle: SSHKeyBundle)] = [] + for id in try await listAll() { + if let bundle = try await load(for: id) { + bundles.append((Self.multiAccountPrefix + id.uuidString, bundle)) + } + } + if let legacy = try? readLegacy() { + bundles.append((Self.legacyV1Account, legacy)) + } + + for (account, bundle) in bundles { + try writeBundle(bundle, account: account, syncToICloud: enabled) + } + } + // MARK: - Private — Keychain plumbing per-account private func readBundle(account: String) throws -> SSHKeyBundle? { let query: [String: Any] = [ - kSecClass as String: kSecClassGenericPassword, - kSecAttrService as String: service, - kSecAttrAccount as String: account, - kSecReturnData as String: true, - kSecMatchLimit as String: kSecMatchLimitOne, + kSecClass as String: kSecClassGenericPassword, + kSecAttrService as String: service, + kSecAttrAccount as String: account, + kSecReturnData as String: true, + kSecMatchLimit as String: kSecMatchLimitOne, + // Match items regardless of sync state (issue #52). Without + // this the query implicitly defaults to false and orphans + // any items that have been migrated to iCloud sync. + kSecAttrSynchronizable as String: kSecAttrSynchronizableAny, ] var item: CFTypeRef? let status = SecItemCopyMatching(query as CFDictionary, &item) @@ -149,6 +208,13 @@ public struct KeychainSSHKeyStore: SSHKeyStore { } private func writeBundle(_ bundle: SSHKeyBundle, account: String) throws { + try writeBundle(bundle, account: account, syncToICloud: SSHKeyICloudPreference.isEnabled) + } + + /// Write path with explicit sync control. Used by the public + /// migration helper to force a target sync state regardless of + /// the current preference. + private func writeBundle(_ bundle: SSHKeyBundle, account: String, syncToICloud: Bool) throws { let data: Data do { data = try JSONEncoder().encode(bundle) @@ -157,17 +223,34 @@ public struct KeychainSSHKeyStore: SSHKeyStore { message: "Encode failed: \(error.localizedDescription)", osStatus: nil ) } - let baseQuery: [String: Any] = [ + // Delete with kSecAttrSynchronizableAny to clear out any prior + // copy regardless of its sync state — without this a flip from + // synced → device-only could leave the synced copy behind and + // create two competing items at the same (service, account). + let deleteQuery: [String: Any] = [ + kSecClass as String: kSecClassGenericPassword, + kSecAttrService as String: service, + kSecAttrAccount as String: account, + kSecAttrSynchronizable as String: kSecAttrSynchronizableAny, + ] + SecItemDelete(deleteQuery as CFDictionary) + + var attributes: [String: Any] = [ kSecClass as String: kSecClassGenericPassword, kSecAttrService as String: service, kSecAttrAccount as String: account, ] - SecItemDelete(baseQuery as CFDictionary) - - var attributes = baseQuery attributes[kSecValueData as String] = data - attributes[kSecAttrAccessible as String] = kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly - attributes[kSecAttrSynchronizable as String] = kCFBooleanFalse + if syncToICloud { + // iCloud Keychain requires the non-`ThisDeviceOnly` accessible + // class — items with the `ThisDeviceOnly` suffix are silently + // skipped by the sync engine. + attributes[kSecAttrAccessible as String] = kSecAttrAccessibleAfterFirstUnlock + attributes[kSecAttrSynchronizable as String] = kCFBooleanTrue + } else { + attributes[kSecAttrAccessible as String] = kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly + attributes[kSecAttrSynchronizable as String] = kCFBooleanFalse + } let addStatus = SecItemAdd(attributes as CFDictionary, nil) guard addStatus == errSecSuccess else { @@ -179,9 +262,10 @@ public struct KeychainSSHKeyStore: SSHKeyStore { private func deleteBundle(account: String) throws { let query: [String: Any] = [ - kSecClass as String: kSecClassGenericPassword, - kSecAttrService as String: service, - kSecAttrAccount as String: account, + kSecClass as String: kSecClassGenericPassword, + kSecAttrService as String: service, + kSecAttrAccount as String: account, + kSecAttrSynchronizable as String: kSecAttrSynchronizableAny, ] let status = SecItemDelete(query as CFDictionary) if status != errSecSuccess && status != errSecItemNotFound { @@ -217,10 +301,13 @@ public struct KeychainSSHKeyStore: SSHKeyStore { /// triggering a recursive migration. private func listAllInternal(skipMigration: Bool) throws -> [ServerID] { let query: [String: Any] = [ - kSecClass as String: kSecClassGenericPassword, - kSecAttrService as String: service, - kSecReturnAttributes as String: true, - kSecMatchLimit as String: kSecMatchLimitAll, + kSecClass as String: kSecClassGenericPassword, + kSecAttrService as String: service, + kSecReturnAttributes as String: true, + kSecMatchLimit as String: kSecMatchLimitAll, + // Match items regardless of sync state (issue #52). Without + // this the listing silently misses synced items. + kSecAttrSynchronizable as String: kSecAttrSynchronizableAny, ] var items: CFTypeRef? let status = SecItemCopyMatching(query as CFDictionary, &items) diff --git a/scarf/Packages/ScarfIOS/Sources/ScarfIOS/SSHKeyICloudPreference.swift b/scarf/Packages/ScarfIOS/Sources/ScarfIOS/SSHKeyICloudPreference.swift new file mode 100644 index 0000000..5d1cb24 --- /dev/null +++ b/scarf/Packages/ScarfIOS/Sources/ScarfIOS/SSHKeyICloudPreference.swift @@ -0,0 +1,39 @@ +// Apple-only: Security.framework + UserDefaults are iOS/Mac only. +// On Linux this file is skipped; tests don't exercise it. +#if canImport(Security) + +import Foundation + +/// Device-local preference: should the SSH key bundle stored in the +/// iOS Keychain sync to iCloud Keychain (issue #52)? +/// +/// **Default `false`.** Existing installs see no change on update; the +/// key remains pinned to the device with `kSecAttrAccessibleAfter +/// FirstUnlockThisDeviceOnly` + `kSecAttrSynchronizable=false`. Users +/// who opt in via Settings → Security trigger a one-shot migration +/// that re-saves all stored keys with `kSecAttrAccessibleAfterFirst +/// Unlock` + `kSecAttrSynchronizable=true` so iCloud Keychain picks +/// them up. +/// +/// **Trade-off the UI must surface clearly.** +/// - On: convenient multi-device — iPhone + iPad + Mac all see the +/// same key. End-to-end encrypted by iCloud Keychain (Apple-managed +/// keys without ADP, user-managed keys with ADP). Requires iCloud +/// Keychain enabled on every device. +/// - Off (default): key never leaves this device. Each device must +/// onboard separately (generate its own key, append its pubkey to +/// `authorized_keys`). +public enum SSHKeyICloudPreference { + + /// UserDefaults key. Stable string so a v2 future fix can read + /// existing values without migration. + public static let key = "scarf.icloud.syncSSHKey" + + /// Read the current preference. Defaults to `false`. + public static var isEnabled: Bool { + get { UserDefaults.standard.bool(forKey: key) } + set { UserDefaults.standard.set(newValue, forKey: key) } + } +} + +#endif // canImport(Security) diff --git a/scarf/Scarf iOS/App/ScarfGoTabRoot.swift b/scarf/Scarf iOS/App/ScarfGoTabRoot.swift index 10add3a..99ca059 100644 --- a/scarf/Scarf iOS/App/ScarfGoTabRoot.swift +++ b/scarf/Scarf iOS/App/ScarfGoTabRoot.swift @@ -138,6 +138,13 @@ private struct SystemTab: View { @State private var showForgetConfirmation = false @State private var isForgetting = false @State private var isDisconnecting = false + /// Mirror of `SSHKeyICloudPreference.isEnabled` — drives the iCloud + /// Keychain sync toggle (issue #52). Initial value is read on view + /// init so the toggle reflects today's preference before the user + /// taps anything; flipping triggers `migrateAllItems(toICloudSync:)`. + @State private var iCloudSyncEnabled: Bool = SSHKeyICloudPreference.isEnabled + @State private var iCloudMigrationInFlight = false + @State private var iCloudMigrationError: String? var body: some View { List { @@ -178,6 +185,67 @@ private struct SystemTab: View { .listRowBackground(ScarfColor.backgroundSecondary) } + Section { + Toggle(isOn: $iCloudSyncEnabled) { + HStack(spacing: 10) { + Image(systemName: "key.icloud.fill") + .foregroundStyle(.tint) + VStack(alignment: .leading, spacing: 2) { + Text("Sync SSH key with iCloud Keychain") + Text(iCloudSyncEnabled + ? "Synced — your other Apple devices with iCloud Keychain will see this key." + : "This device only — generate a separate key on each device.") + .font(.caption) + .foregroundStyle(ScarfColor.foregroundMuted) + } + } + } + .tint(ScarfColor.accent) + .disabled(iCloudMigrationInFlight) + .onChange(of: iCloudSyncEnabled) { _, newValue in + Task { + iCloudMigrationInFlight = true + iCloudMigrationError = nil + defer { iCloudMigrationInFlight = false } + do { + try await KeychainSSHKeyStore().migrateAllItems(toICloudSync: newValue) + } catch { + // Revert the toggle on failure so the UI + // reflects what's actually in the Keychain; + // surface the error inline so the user can + // retry / report. Keychain failures here are + // rare (typically `errSecDuplicateItem` if a + // prior migration was interrupted — the + // delete-with-Any in writeBundle prevents + // that, but we still belt-and-brace). + iCloudMigrationError = error.localizedDescription + iCloudSyncEnabled = !newValue + SSHKeyICloudPreference.isEnabled = !newValue + } + } + } + if iCloudMigrationInFlight { + HStack(spacing: 8) { + ProgressView() + .controlSize(.small) + Text("Updating Keychain…") + .font(.caption) + .foregroundStyle(ScarfColor.foregroundMuted) + } + } + if let err = iCloudMigrationError { + Label(err, systemImage: "exclamationmark.triangle.fill") + .font(.caption) + .foregroundStyle(ScarfColor.warning) + } + } header: { + Text("Security") + } footer: { + Text("End-to-end encrypted via iCloud Keychain. With Advanced Data Protection on, the encryption keys never leave your devices. Toggle off to keep the key device-only — each new device must onboard separately.") + .font(.caption) + } + .listRowBackground(ScarfColor.backgroundSecondary) + Section { Button { Task {