diff --git a/mac-app/Sources/CloudMachineCore/DriveEncryption.swift b/mac-app/Sources/CloudMachineCore/DriveEncryption.swift index 0b31f0a..a9966cd 100644 --- a/mac-app/Sources/CloudMachineCore/DriveEncryption.swift +++ b/mac-app/Sources/CloudMachineCore/DriveEncryption.swift @@ -555,6 +555,15 @@ public enum DriveEncryption { return Data(bytes).base64EncodedString() } + /// What `setup` keeps in the Keychain: the password and the salt exactly as + /// `obscure` encrypted them. A trimmed copy of a password with an edge + /// space would not decrypt the backup. + static func keychainItems(password: String, salt: String) + -> [(account: String, value: String, exact: Bool)] + { + [(passwordAccount, password, true), (saltAccount, salt, true)] + } + /// `rclone obscure`, the secret through the environment (not `ps`). static func obscure(_ secret: String) async -> String? { let result = try? await ProcessRunner.run( @@ -608,8 +617,10 @@ public enum DriveEncryption { // the one mistake here that cannot be undone. let service = keychainService(instance: VMLayout.instanceName) do { - try await KeychainStore.save(password, account: passwordAccount, service: service) - try await KeychainStore.save(salt, account: saltAccount, service: service) + for item in keychainItems(password: password, salt: salt) { + try await KeychainStore.save( + item.value, account: item.account, service: service, exact: item.exact) + } } catch { return fail(error.localizedDescription) } diff --git a/mac-app/Sources/CloudMachineCore/KeychainStore.swift b/mac-app/Sources/CloudMachineCore/KeychainStore.swift index 09d460b..1703d3b 100644 --- a/mac-app/Sources/CloudMachineCore/KeychainStore.swift +++ b/mac-app/Sources/CloudMachineCore/KeychainStore.swift @@ -37,19 +37,32 @@ public enum KeychainStore { /// Saves or overwrites an item. `-U` means "replace if it already exists" - /// without it, fixing a typo would end in an error with the old value still /// in use. - public static func save(_ value: String, account: String, service: String) async throws { - let trimmed = value.trimmingCharacters(in: .whitespacesAndNewlines) - guard !trimmed.isEmpty else { throw StoreError.emptyValue } + /// + /// `exact`: store the value as given. For a secret that is used elsewhere + /// untrimmed - the encryption password goes to `rclone obscure` exactly as + /// typed, so a trimmed Keychain copy would not decrypt the backup. + public static func save( + _ value: String, account: String, service: String, exact: Bool = false + ) async throws { + let stored = try valueToStore(value, exact: exact) let result = try? await ProcessRunner.run( "/usr/bin/security", - ["add-generic-password", "-a", account, "-s", service, "-w", trimmed, "-U"], + ["add-generic-password", "-a", account, "-s", service, "-w", stored, "-U"], timeout: 30) guard result?.succeeded == true else { throw StoreError.failed(result?.stderr ?? L10n.tr("unknown error")) } } + /// The value `save` hands to `security`: trimmed (pasted keys carry + /// newlines), or as given with `exact`. Blank is refused either way. + static func valueToStore(_ value: String, exact: Bool = false) throws -> String { + let trimmed = value.trimmingCharacters(in: .whitespacesAndNewlines) + guard !trimmed.isEmpty else { throw StoreError.emptyValue } + return exact ? value : trimmed + } + /// Whether the item exists - WITHOUT `-w`, i.e. without reaching for the /// value itself. /// diff --git a/mac-app/Tests/CloudMachineAppTests/KeychainStoreValueTests.swift b/mac-app/Tests/CloudMachineAppTests/KeychainStoreValueTests.swift new file mode 100644 index 0000000..43de24a --- /dev/null +++ b/mac-app/Tests/CloudMachineAppTests/KeychainStoreValueTests.swift @@ -0,0 +1,46 @@ +import XCTest + +@testable import CloudMachineCore + +/// What `KeychainStore.save` actually hands to `security` - without touching +/// this Mac's Keychain. +/// +/// The encryption password goes to `rclone obscure` exactly as typed. If the +/// Keychain copy were trimmed, a password with a leading or trailing space +/// would be stored DIFFERENT from the one that encrypts the backup, and the +/// copy meant for recovery would not decrypt it. +final class KeychainStoreValueTests: XCTestCase { + + func testExactValueKeepsLeadingAndTrailingSpaces() throws { + XCTAssertEqual( + try KeychainStore.valueToStore(" pass phrase with spaces ", exact: true), + " pass phrase with spaces ") + } + + func testDefaultTrimsPastedNewlinesAndSpaces() throws { + // A pasted client_id / client_secret often carries a newline. + XCTAssertEqual(try KeychainStore.valueToStore(" client-id\n"), "client-id") + } + + func testEmptyIsRefusedInBothModes() { + XCTAssertThrowsError(try KeychainStore.valueToStore(" \n")) + XCTAssertThrowsError(try KeychainStore.valueToStore("", exact: true)) + } + + /// What `DriveEncryption.setup` stores is what `obscure` encrypted with: + /// the value as `KeychainStore.save` would hand it to `security`. + func testEncryptionSetupStoresPasswordAndSaltExactly() throws { + let password = " edge spaces password " + let salt = "c2FsdA==" + let stored = try DriveEncryption.keychainItems(password: password, salt: salt).map { + ($0.account, try KeychainStore.valueToStore($0.value, exact: $0.exact)) + } + XCTAssertEqual(stored.map(\.0), [DriveEncryption.passwordAccount, DriveEncryption.saltAccount]) + XCTAssertEqual(stored.map(\.1), [password, salt]) + } + + func testExactWhitespaceOnlyIsRefused() { + // Nothing meaningful to keep - same refusal as before. + XCTAssertThrowsError(try KeychainStore.valueToStore(" ", exact: true)) + } +}