Fix for SecretAgent acting as if it has no keys after updating macOS (#427)

* Allow reload pre-op

* Fix tests

* Make sure standin keys get rewritten on force update

* Stub store
This commit is contained in:
Max Goedjen 2022-12-17 23:16:56 -08:00 committed by GitHub
parent 382913cb99
commit 3b254d33a5
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
7 changed files with 70 additions and 31 deletions

View File

@ -31,13 +31,14 @@ public class Agent {
private let writer = OpenSSHKeyWriter() private let writer = OpenSSHKeyWriter()
private let requestTracer = SigningRequestTracer() private let requestTracer = SigningRequestTracer()
private let certsPath = (NSHomeDirectory() as NSString).appendingPathComponent("PublicKeys") as String private let certsPath = (NSHomeDirectory() as NSString).appendingPathComponent("PublicKeys") as String
private let logger = Logger(subsystem: "com.maxgoedjen.secretive.secretagent.agent", category: "")
/// Initializes an agent with a store list and a witness. /// Initializes an agent with a store list and a witness.
/// - Parameters: /// - Parameters:
/// - storeList: The `SecretStoreList` to make available. /// - storeList: The `SecretStoreList` to make available.
/// - witness: A witness to notify of requests. /// - witness: A witness to notify of requests.
public init(storeList: SecretStoreList, witness: SigningWitness? = nil) { public init(storeList: SecretStoreList, witness: SigningWitness? = nil) {
Logger().debug("Agent is running") logger.debug("Agent is running")
self.storeList = storeList self.storeList = storeList
self.witness = witness self.witness = witness
} }
@ -53,16 +54,16 @@ extension Agent {
/// - Return value: /// - Return value:
/// - Boolean if data could be read /// - Boolean if data could be read
@discardableResult public func handle(reader: FileHandleReader, writer: FileHandleWriter) -> Bool { @discardableResult public func handle(reader: FileHandleReader, writer: FileHandleWriter) -> Bool {
Logger().debug("Agent handling new data") logger.debug("Agent handling new data")
let data = Data(reader.availableData) let data = Data(reader.availableData)
guard data.count > 4 else { return false} guard data.count > 4 else { return false}
let requestTypeInt = data[4] let requestTypeInt = data[4]
guard let requestType = SSHAgent.RequestType(rawValue: requestTypeInt) else { guard let requestType = SSHAgent.RequestType(rawValue: requestTypeInt) else {
writer.write(OpenSSHKeyWriter().lengthAndData(of: SSHAgent.ResponseType.agentFailure.data)) writer.write(OpenSSHKeyWriter().lengthAndData(of: SSHAgent.ResponseType.agentFailure.data))
Logger().debug("Agent returned \(SSHAgent.ResponseType.agentFailure.debugDescription)") logger.debug("Agent returned \(SSHAgent.ResponseType.agentFailure.debugDescription)")
return true return true
} }
Logger().debug("Agent handling request of type \(requestType.debugDescription)") logger.debug("Agent handling request of type \(requestType.debugDescription)")
let subData = Data(data[5...]) let subData = Data(data[5...])
let response = handle(requestType: requestType, data: subData, reader: reader) let response = handle(requestType: requestType, data: subData, reader: reader)
writer.write(response) writer.write(response)
@ -70,23 +71,25 @@ extension Agent {
} }
func handle(requestType: SSHAgent.RequestType, data: Data, reader: FileHandleReader) -> Data { func handle(requestType: SSHAgent.RequestType, data: Data, reader: FileHandleReader) -> Data {
// Depending on the launch context (such as after macOS update), the agent may need to reload secrets before acting
reloadSecretsIfNeccessary()
var response = Data() var response = Data()
do { do {
switch requestType { switch requestType {
case .requestIdentities: case .requestIdentities:
response.append(SSHAgent.ResponseType.agentIdentitiesAnswer.data) response.append(SSHAgent.ResponseType.agentIdentitiesAnswer.data)
response.append(identities()) response.append(identities())
Logger().debug("Agent returned \(SSHAgent.ResponseType.agentIdentitiesAnswer.debugDescription)") logger.debug("Agent returned \(SSHAgent.ResponseType.agentIdentitiesAnswer.debugDescription)")
case .signRequest: case .signRequest:
let provenance = requestTracer.provenance(from: reader) let provenance = requestTracer.provenance(from: reader)
response.append(SSHAgent.ResponseType.agentSignResponse.data) response.append(SSHAgent.ResponseType.agentSignResponse.data)
response.append(try sign(data: data, provenance: provenance)) response.append(try sign(data: data, provenance: provenance))
Logger().debug("Agent returned \(SSHAgent.ResponseType.agentSignResponse.debugDescription)") logger.debug("Agent returned \(SSHAgent.ResponseType.agentSignResponse.debugDescription)")
} }
} catch { } catch {
response.removeAll() response.removeAll()
response.append(SSHAgent.ResponseType.agentFailure.data) response.append(SSHAgent.ResponseType.agentFailure.data)
Logger().debug("Agent returned \(SSHAgent.ResponseType.agentFailure.debugDescription)") logger.debug("Agent returned \(SSHAgent.ResponseType.agentFailure.debugDescription)")
} }
let full = OpenSSHKeyWriter().lengthAndData(of: response) let full = OpenSSHKeyWriter().lengthAndData(of: response)
return full return full
@ -120,7 +123,7 @@ extension Agent {
keyData.append(writer.lengthAndData(of: curveData)) keyData.append(writer.lengthAndData(of: curveData))
} }
Logger().debug("Agent enumerated \(secrets.count) identities") logger.log("Agent enumerated \(secrets.count) identities")
return countData + keyData return countData + keyData
} }
@ -139,7 +142,7 @@ extension Agent {
} }
guard let (store, secret) = secret(matching: hash) else { guard let (store, secret) = secret(matching: hash) else {
Logger().debug("Agent did not have a key matching \(hash as NSData)") logger.debug("Agent did not have a key matching \(hash as NSData)")
throw AgentError.noMatchingKey throw AgentError.noMatchingKey
} }
@ -193,7 +196,7 @@ extension Agent {
try witness.witness(accessTo: secret, from: store, by: provenance) try witness.witness(accessTo: secret, from: store, by: provenance)
} }
Logger().debug("Agent signed request") logger.debug("Agent signed request")
return signedData return signedData
} }
@ -235,7 +238,7 @@ extension Agent {
let certificatePath = certsPath.appending("/").appending("\(minimalHex)-cert.pub") let certificatePath = certsPath.appending("/").appending("\(minimalHex)-cert.pub")
if FileManager.default.fileExists(atPath: certificatePath) { if FileManager.default.fileExists(atPath: certificatePath) {
Logger().debug("Found certificate for \(secret.name)") logger.debug("Found certificate for \(secret.name)")
do { do {
let certContent = try String(contentsOfFile:certificatePath, encoding: .utf8) let certContent = try String(contentsOfFile:certificatePath, encoding: .utf8)
let certElements = certContent.trimmingCharacters(in: .whitespacesAndNewlines).components(separatedBy: " ") let certElements = certContent.trimmingCharacters(in: .whitespacesAndNewlines).components(separatedBy: " ")
@ -246,19 +249,19 @@ extension Agent {
if let certName = certElements[2].data(using: .utf8) { if let certName = certElements[2].data(using: .utf8) {
return (certDecoded, certName) return (certDecoded, certName)
} else if let certName = secret.name.data(using: .utf8) { } else if let certName = secret.name.data(using: .utf8) {
Logger().info("Certificate for \(secret.name) does not have a name tag, using secret name instead") logger.info("Certificate for \(secret.name) does not have a name tag, using secret name instead")
return (certDecoded, certName) return (certDecoded, certName)
} else { } else {
throw OpenSSHCertificateError.parsingFailed throw OpenSSHCertificateError.parsingFailed
} }
} }
} else { } else {
Logger().warning("Certificate found for \(secret.name) but failed to decode base64 key") logger.warning("Certificate found for \(secret.name) but failed to decode base64 key")
throw OpenSSHCertificateError.parsingFailed throw OpenSSHCertificateError.parsingFailed
} }
} }
} catch { } catch {
Logger().warning("Certificate found for \(secret.name) but failed to load") logger.warning("Certificate found for \(secret.name) but failed to load")
throw OpenSSHCertificateError.parsingFailed throw OpenSSHCertificateError.parsingFailed
} }
} }
@ -270,6 +273,16 @@ extension Agent {
extension Agent { extension Agent {
/// Gives any store with no loaded secrets a chance to reload.
func reloadSecretsIfNeccessary() {
for store in storeList.stores {
if store.secrets.isEmpty {
logger.debug("Store \(store.name, privacy: .public) has no loaded secrets. Reloading.")
store.reloadSecrets()
}
}
}
/// Finds a ``Secret`` matching a specified hash whos signature was requested. /// Finds a ``Secret`` matching a specified hash whos signature was requested.
/// - Parameter hash: The hash to match against. /// - Parameter hash: The hash to match against.
/// - Returns: A ``Secret`` and the ``SecretStore`` containing it, if a match is found. /// - Returns: A ``Secret`` and the ``SecretStore`` containing it, if a match is found.

View File

@ -12,6 +12,7 @@ public class AnySecretStore: SecretStore {
private let _sign: (Data, AnySecret, SigningRequestProvenance) throws -> Data private let _sign: (Data, AnySecret, SigningRequestProvenance) throws -> Data
private let _existingPersistedAuthenticationContext: (AnySecret) -> PersistedAuthenticationContext? private let _existingPersistedAuthenticationContext: (AnySecret) -> PersistedAuthenticationContext?
private let _persistAuthentication: (AnySecret, TimeInterval) throws -> Void private let _persistAuthentication: (AnySecret, TimeInterval) throws -> Void
private let _reloadSecrets: () -> Void
private var sink: AnyCancellable? private var sink: AnyCancellable?
@ -24,6 +25,7 @@ public class AnySecretStore: SecretStore {
_sign = { try secretStore.sign(data: $0, with: $1.base as! SecretStoreType.SecretType, for: $2) } _sign = { try secretStore.sign(data: $0, with: $1.base as! SecretStoreType.SecretType, for: $2) }
_existingPersistedAuthenticationContext = { secretStore.existingPersistedAuthenticationContext(secret: $0.base as! SecretStoreType.SecretType) } _existingPersistedAuthenticationContext = { secretStore.existingPersistedAuthenticationContext(secret: $0.base as! SecretStoreType.SecretType) }
_persistAuthentication = { try secretStore.persistAuthentication(secret: $0.base as! SecretStoreType.SecretType, forDuration: $1) } _persistAuthentication = { try secretStore.persistAuthentication(secret: $0.base as! SecretStoreType.SecretType, forDuration: $1) }
_reloadSecrets = { secretStore.reloadSecrets() }
sink = secretStore.objectWillChange.sink { _ in sink = secretStore.objectWillChange.sink { _ in
self.objectWillChange.send() self.objectWillChange.send()
} }
@ -57,6 +59,10 @@ public class AnySecretStore: SecretStore {
try _persistAuthentication(secret, duration) try _persistAuthentication(secret, duration)
} }
public func reloadSecrets() {
_reloadSecrets()
}
} }
public class AnySecretStoreModifiable: AnySecretStore, SecretStoreModifiable { public class AnySecretStoreModifiable: AnySecretStore, SecretStoreModifiable {

View File

@ -36,6 +36,9 @@ public protocol SecretStore: ObservableObject, Identifiable {
/// - Note: This is used for temporarily unlocking access to a secret which would otherwise require authentication every single use. This is useful for situations where the user anticipates several rapid accesses to a authorization-guarded secret. /// - Note: This is used for temporarily unlocking access to a secret which would otherwise require authentication every single use. This is useful for situations where the user anticipates several rapid accesses to a authorization-guarded secret.
func persistAuthentication(secret: SecretType, forDuration duration: TimeInterval) throws func persistAuthentication(secret: SecretType, forDuration duration: TimeInterval) throws
/// Requests that the store reload secrets from any backing store, if neccessary.
func reloadSecrets()
} }
/// A SecretStore that the Secretive admin app can modify. /// A SecretStore that the Secretive admin app can modify.

View File

@ -24,7 +24,7 @@ extension SecureEnclave {
/// Initializes a Store. /// Initializes a Store.
public init() { public init() {
DistributedNotificationCenter.default().addObserver(forName: .secretStoreUpdated, object: nil, queue: .main) { _ in DistributedNotificationCenter.default().addObserver(forName: .secretStoreUpdated, object: nil, queue: .main) { _ in
self.reloadSecrets(notifyAgent: false) self.reloadSecretsInternal(notifyAgent: false)
} }
loadSecrets() loadSecrets()
} }
@ -68,7 +68,7 @@ extension SecureEnclave {
throw KeychainError(statusCode: nil) throw KeychainError(statusCode: nil)
} }
try savePublicKey(publicKey, name: name) try savePublicKey(publicKey, name: name)
reloadSecrets() reloadSecretsInternal()
} }
public func delete(secret: Secret) throws { public func delete(secret: Secret) throws {
@ -80,7 +80,7 @@ extension SecureEnclave {
if status != errSecSuccess { if status != errSecSuccess {
throw KeychainError(statusCode: status) throw KeychainError(statusCode: status)
} }
reloadSecrets() reloadSecretsInternal()
} }
public func update(secret: Secret, name: String) throws { public func update(secret: Secret, name: String) throws {
@ -97,7 +97,7 @@ extension SecureEnclave {
if status != errSecSuccess { if status != errSecSuccess {
throw KeychainError(statusCode: status) throw KeychainError(statusCode: status)
} }
reloadSecrets() reloadSecretsInternal()
} }
public func sign(data: Data, with secret: SecretType, for provenance: SigningRequestProvenance) throws -> Data { public func sign(data: Data, with secret: SecretType, for provenance: SigningRequestProvenance) throws -> Data {
@ -163,6 +163,10 @@ extension SecureEnclave {
} }
} }
public func reloadSecrets() {
reloadSecretsInternal(notifyAgent: false)
}
} }
} }
@ -171,12 +175,15 @@ extension SecureEnclave.Store {
/// Reloads all secrets from the store. /// Reloads all secrets from the store.
/// - Parameter notifyAgent: A boolean indicating whether a distributed notification should be posted, notifying other processes (ie, the SecretAgent) to reload their stores as well. /// - Parameter notifyAgent: A boolean indicating whether a distributed notification should be posted, notifying other processes (ie, the SecretAgent) to reload their stores as well.
private func reloadSecrets(notifyAgent: Bool = true) { private func reloadSecretsInternal(notifyAgent: Bool = true) {
let before = secrets
secrets.removeAll() secrets.removeAll()
loadSecrets() loadSecrets()
NotificationCenter.default.post(name: .secretStoreReloaded, object: self) if secrets != before {
if notifyAgent { NotificationCenter.default.post(name: .secretStoreReloaded, object: self)
DistributedNotificationCenter.default().postNotificationName(.secretStoreUpdated, object: nil, deliverImmediately: true) if notifyAgent {
DistributedNotificationCenter.default().postNotificationName(.secretStoreUpdated, object: nil, deliverImmediately: true)
}
} }
} }

View File

@ -89,6 +89,19 @@ extension SmartCard {
public func persistAuthentication(secret: SmartCard.Secret, forDuration: TimeInterval) throws { public func persistAuthentication(secret: SmartCard.Secret, forDuration: TimeInterval) throws {
} }
/// Reloads all secrets from the store.
public func reloadSecrets() {
DispatchQueue.main.async {
self.isAvailable = self.tokenID != nil
let before = self.secrets
self.secrets.removeAll()
self.loadSecrets()
if self.secrets != before {
NotificationCenter.default.post(name: .secretStoreReloaded, object: self)
}
}
}
} }
} }
@ -102,15 +115,6 @@ extension SmartCard.Store {
reloadSecrets() reloadSecrets()
} }
/// Reloads all secrets from the store.
private func reloadSecrets() {
DispatchQueue.main.async {
self.isAvailable = self.tokenID != nil
self.secrets.removeAll()
self.loadSecrets()
}
}
/// Loads all secrets from the store. /// Loads all secrets from the store.
private func loadSecrets() { private func loadSecrets() {
guard let tokenID = tokenID else { return } guard let tokenID = tokenID else { return }

View File

@ -78,6 +78,9 @@ extension Stub {
public func persistAuthentication(secret: Stub.Secret, forDuration duration: TimeInterval) throws { public func persistAuthentication(secret: Stub.Secret, forDuration duration: TimeInterval) throws {
} }
public func reloadSecrets() {
}
} }
} }

View File

@ -47,6 +47,9 @@ extension Preview {
func persistAuthentication(secret: Preview.Secret, forDuration duration: TimeInterval) throws { func persistAuthentication(secret: Preview.Secret, forDuration duration: TimeInterval) throws {
} }
func reloadSecrets() {
}
} }
class StoreModifiable: Store, SecretStoreModifiable { class StoreModifiable: Store, SecretStoreModifiable {