From 8202ebcf01541d966df1d38c20b547f33344ce26 Mon Sep 17 00:00:00 2001 From: Erik Doernenburg Date: Tue, 21 May 2024 10:23:06 +0200 Subject: [PATCH] Made keychain a singleton and added cache as workaround for macOS bug. The SecItemCopyMatching function leaks on macOS. This has been known for years, and the bug is reported, but who knows when Apple fix it. --- CCMenu/Source/Miscellaneous/Keychain.swift | 122 +++++++++++------- .../CCTray Sheets/CCTrayPipelineBuilder.swift | 2 +- .../GitHub Sheets/GitHubAuthenticator.swift | 4 +- .../Server Monitor/CCTrayFeedReader.swift | 2 +- .../Server Monitor/GitHubFeedReader.swift | 2 +- 5 files changed, 83 insertions(+), 49 deletions(-) diff --git a/CCMenu/Source/Miscellaneous/Keychain.swift b/CCMenu/Source/Miscellaneous/Keychain.swift index 2ba5516..adee6ea 100644 --- a/CCMenu/Source/Miscellaneous/Keychain.swift +++ b/CCMenu/Source/Miscellaneous/Keychain.swift @@ -14,88 +14,122 @@ enum KeychainAccessError: Error { case missingSchemeErr case missingHostErr case missingUserErr + case invalidKeychainType } class Keychain { + static let standard = Keychain() + + private let lock: NSLock + private var cache: [String: String] + + private init() { + lock = NSLock() + cache = Dictionary() + } + func setPassword(_ password: String, forURL urlString: String) throws { let url = try getOrThrow(error: .invalidURLErr) { URL(string: urlString) } - let query: [String: Any] = [ - kSecClass as String: kSecClassInternetPassword, - kSecAttrServer as String: try getOrThrow(error: .missingHostErr) { url.host() }, - kSecAttrPort as String: url.port ?? 80, - kSecAttrAccount as String: try getOrThrow(error: .missingUserErr) { url.user } - ] - var item: [String: Any] = [ - kSecAttrProtocol as String: try getOrThrow(error: .missingSchemeErr) { url.scheme }, - kSecValueData as String: try getOrThrow(error: .passwordEncodingErr) { password.data(using: .utf8) } - ] - item.merge(query) { i, q in i } + let query = [ + kSecClass: kSecClassInternetPassword, + kSecAttrServer: try getOrThrow(error: .missingHostErr) { url.host() }, + kSecAttrPort: url.port ?? 80, + kSecAttrAccount: try getOrThrow(error: .missingUserErr) { url.user } + ] as NSDictionary + let item = [ + kSecClass: kSecClassInternetPassword, + kSecAttrServer: try getOrThrow(error: .missingHostErr) { url.host() }, + kSecAttrPort: url.port ?? 80, + kSecAttrAccount: try getOrThrow(error: .missingUserErr) { url.user }, + kSecAttrProtocol: try getOrThrow(error: .missingSchemeErr) { url.scheme }, + kSecValueData: try getOrThrow(error: .passwordEncodingErr) { password.data(using: .utf8) } + ] as NSDictionary try setItem(item, forQuery: query) + cache[urlString] = nil } func getPassword(forURL url: URL) throws -> String? { - let query: [String: Any] = [ - kSecClass as String: kSecClassInternetPassword, - kSecAttrServer as String: try getOrThrow(error: .missingHostErr) { url.host() }, - kSecAttrPort as String: url.port ?? 80, - kSecAttrAccount as String: try getOrThrow(error: .missingUserErr) { url.user }, - kSecReturnData as String: true - ] - return try getStringForQuery(query) + if let password = cache[url.absoluteString] { + return password + } + let query = [ + kSecClass: kSecClassInternetPassword, + kSecAttrServer: try getOrThrow(error: .missingHostErr) { url.host() }, + kSecAttrPort: url.port ?? 80, + kSecAttrAccount: try getOrThrow(error: .missingUserErr) { url.user }, + kSecMatchLimit: kSecMatchLimitOne, + kSecReturnData: true + ] as NSDictionary + let password = try getStringForQuery(query) + cache[url.absoluteString] = password + return password } func setToken(_ token: String, forService service: String) throws { - let query: [String: Any] = [ - kSecClass as String: kSecClassGenericPassword, - kSecAttrService as String: serviceForKeychain(service: service) - ] - var item: [String: Any] = [ - kSecValueData as String: try getOrThrow(error: .passwordEncodingErr) { token.data(using: .utf8) } - ] - item.merge(query) { i, q in i } + let query = [ + kSecClass: kSecClassGenericPassword, + kSecAttrService: serviceForKeychain(service: service) + ] as NSDictionary + let item = [ + kSecClass: kSecClassGenericPassword, + kSecAttrService: serviceForKeychain(service: service), + kSecValueData: try getOrThrow(error: .passwordEncodingErr) { token.data(using: .utf8) } + ] as NSDictionary try setItem(item, forQuery: query) + cache[service] = nil } func getToken(forService service: String) throws -> String? { if service == "GitHub", let token = UserDefaults.active.string(forKey: "GitHubToken") { return token.isEmpty ? nil : token } - let query: [String: Any] = [ - kSecClass as String: kSecClassGenericPassword, - kSecAttrService as String: serviceForKeychain(service: service), - kSecReturnData as String: true - ] - return try getStringForQuery(query) - } - + if let token = cache[service] { + return token + } + let query = [ + kSecClass: kSecClassGenericPassword, + kSecAttrService: serviceForKeychain(service: service), + kSecMatchLimit: kSecMatchLimitOne, + kSecReturnData: true + ] as NSDictionary + let token = try getStringForQuery(query) + cache[service] = token + return token + } + - private func setItem(_ item: [String: Any], forQuery query: [String: Any]) throws { - var status = SecItemAdd(item as CFDictionary, nil) + private func setItem(_ item: NSDictionary, forQuery query: NSDictionary) throws { + lock.lock() + defer { lock.unlock() } + + var status = SecItemAdd(item, nil) if status == errSecDuplicateItem { - status = SecItemUpdate(query as CFDictionary, item as CFDictionary) + status = SecItemUpdate(query, item) } if status != errSecSuccess { throw NSError(domain: NSOSStatusErrorDomain, code: Int(status)) } } - private func getStringForQuery(_ query: [String: Any]) throws -> String? { - var result: AnyObject? - let status = withUnsafeMutablePointer(to: &result) { - SecItemCopyMatching(query as CFDictionary, UnsafeMutablePointer($0)) - } + private func getStringForQuery(_ query: NSDictionary) throws -> String? { + lock.lock() + defer { lock.unlock() } + + var result: CFTypeRef? + let status = SecItemCopyMatching(query, &result) if status == errSecItemNotFound { return nil } if status != errSecSuccess { throw NSError(domain: NSOSStatusErrorDomain, code: Int(status)) } - guard let data = result as? Data else { return nil } + guard let data = result as? Data else { throw KeychainAccessError.invalidKeychainType } return String(data: data, encoding: .utf8) } + private func getOrThrow(error: KeychainAccessError, _ getter: () -> T?) throws -> T { guard let v = getter() else { throw error } return v diff --git a/CCMenu/Source/Pipeline Window/CCTray Sheets/CCTrayPipelineBuilder.swift b/CCMenu/Source/Pipeline Window/CCTray Sheets/CCTrayPipelineBuilder.swift index eff6b46..0de9901 100644 --- a/CCMenu/Source/Pipeline Window/CCTray Sheets/CCTrayPipelineBuilder.swift +++ b/CCMenu/Source/Pipeline Window/CCTray Sheets/CCTrayPipelineBuilder.swift @@ -39,7 +39,7 @@ class CCTrayPipelineBuilder: ObservableObject { components.user = credential.user if !credential.password.isEmpty { do { - try Keychain().setPassword(credential.password, forURL: url.absoluteString) + try Keychain.standard.setPassword(credential.password, forURL: url.absoluteString) } catch { // TODO: Figure out what to do here – so many errors... } diff --git a/CCMenu/Source/Pipeline Window/GitHub Sheets/GitHubAuthenticator.swift b/CCMenu/Source/Pipeline Window/GitHub Sheets/GitHubAuthenticator.swift index fccbdf7..104c98a 100644 --- a/CCMenu/Source/Pipeline Window/GitHub Sheets/GitHubAuthenticator.swift +++ b/CCMenu/Source/Pipeline Window/GitHub Sheets/GitHubAuthenticator.swift @@ -121,7 +121,7 @@ class GitHubAuthenticator: ObservableObject { func fetchTokenFromKeychain() { do { - token = try Keychain().getToken(forService: "GitHub") + token = try Keychain.standard.getToken(forService: "GitHub") } catch { // TODO: Figure out what to do here – so many errors... token = nil @@ -133,7 +133,7 @@ class GitHubAuthenticator: ObservableObject { func storeTokenInKeychain() { guard let token else { return } do { - try Keychain().setToken(token, forService: "GitHub") + try Keychain.standard.setToken(token, forService: "GitHub") } catch { // TODO: Figure out what to do here – so many errors... } diff --git a/CCMenu/Source/Server Monitor/CCTrayFeedReader.swift b/CCMenu/Source/Server Monitor/CCTrayFeedReader.swift index 708b943..383f808 100644 --- a/CCMenu/Source/Server Monitor/CCTrayFeedReader.swift +++ b/CCMenu/Source/Server Monitor/CCTrayFeedReader.swift @@ -52,7 +52,7 @@ class CCTrayFeedReader { func requestForFeed(feed: Pipeline.Feed) throws -> URLRequest { var credential: HTTPCredential? if let user = feed.url.user() { - guard let password = try Keychain().getPassword(forURL: feed.url) else { + guard let password = try Keychain.standard.getPassword(forURL: feed.url) else { throw CCTrayFeedReaderError.missingPasswordError } credential = HTTPCredential(user: user, password: password) diff --git a/CCMenu/Source/Server Monitor/GitHubFeedReader.swift b/CCMenu/Source/Server Monitor/GitHubFeedReader.swift index 9f57b29..2188e31 100644 --- a/CCMenu/Source/Server Monitor/GitHubFeedReader.swift +++ b/CCMenu/Source/Server Monitor/GitHubFeedReader.swift @@ -38,7 +38,7 @@ class GitHubFeedReader { func updatePipelineStatus() async { do { - let token = try Keychain().getToken(forService: "GitHub") + let token = try Keychain.standard.getToken(forService: "GitHub") guard let request = GitHubAPI.requestForFeed(feed: pipeline.feed, token: token) else { throw GithHubFeedReaderError.invalidURLError }