From f43571baa3e4821398180d29663974d2f2c2b476 Mon Sep 17 00:00:00 2001 From: Max Goedjen Date: Wed, 21 Dec 2022 20:18:27 -0800 Subject: [PATCH] SSH Certificate Cleanup/Followup (#418) * Don't delete public cert keys corrresponding to secretive-tracked keys * First pass * Fix fallback for name * Split out into dedicated handler * Stub add identities * . --- .../Sources/SecretAgentKit/Agent.swift | 110 ++-------------- .../SecretAgentKit/SSHAgentProtocol.swift | 9 +- .../OpenSSH/OpenSSHCertificateHandler.swift | 120 ++++++++++++++++++ .../PublicKeyStandinFileController.swift | 12 ++ .../Sources/SecretKit/SecretStoreList.swift | 4 + Sources/SecretAgent/AppDelegate.swift | 4 +- 6 files changed, 158 insertions(+), 101 deletions(-) create mode 100644 Sources/Packages/Sources/SecretKit/OpenSSH/OpenSSHCertificateHandler.swift diff --git a/Sources/Packages/Sources/SecretAgentKit/Agent.swift b/Sources/Packages/Sources/SecretAgentKit/Agent.swift index ce1c8b4..84cd238 100644 --- a/Sources/Packages/Sources/SecretAgentKit/Agent.swift +++ b/Sources/Packages/Sources/SecretAgentKit/Agent.swift @@ -4,25 +4,6 @@ import OSLog import SecretKit import AppKit -enum OpenSSHCertificateError: Error { - case unsupportedType - case parsingFailed - case doesNotExist -} - -extension OpenSSHCertificateError: CustomStringConvertible { - public var description: String { - switch self { - case .unsupportedType: - return "The key type was unsupported" - case .parsingFailed: - return "Failed to properly parse the SSH certificate" - case .doesNotExist: - return "Certificate does not exist" - } - } -} - /// The `Agent` is an implementation of an SSH agent. It manages coordination and access between a socket, traces requests, notifies witnesses and passes requests to stores. public class Agent { @@ -30,7 +11,7 @@ public class Agent { private let witness: SigningWitness? private let writer = OpenSSHKeyWriter() private let requestTracer = SigningRequestTracer() - private let certsPath = (NSHomeDirectory() as NSString).appendingPathComponent("PublicKeys") as String + private let certificateHandler = OpenSSHCertificateHandler() private let logger = Logger(subsystem: "com.maxgoedjen.secretive.secretagent.agent", category: "") /// Initializes an agent with a store list and a witness. @@ -41,6 +22,7 @@ public class Agent { logger.debug("Agent is running") self.storeList = storeList self.witness = witness + certificateHandler.reloadCertificates(for: storeList.allSecrets) } } @@ -102,7 +84,8 @@ extension Agent { /// Lists the identities available for signing operations /// - Returns: An OpenSSH formatted Data payload listing the identities available for signing operations. func identities() -> Data { - let secrets = storeList.stores.flatMap(\.secrets) + let secrets = storeList.allSecrets + certificateHandler.reloadCertificates(for: secrets) var count = UInt32(secrets.count).bigEndian let countData = Data(bytes: &count, count: UInt32.bitWidth/8) var keyData = Data() @@ -111,9 +94,9 @@ extension Agent { let keyBlob: Data let curveData: Data - if let (certBlob, certName) = try? checkForCert(secret: secret) { - keyBlob = certBlob - curveData = certName + if let (certificateData, name) = try? certificateHandler.keyBlobAndName(for: secret) { + keyBlob = certificateData + curveData = name } else { keyBlob = writer.data(secret: secret) curveData = writer.curveType(for: secret.algorithm, length: secret.keySize).data(using: .utf8)! @@ -134,11 +117,13 @@ extension Agent { /// - Returns: An OpenSSH formatted Data payload containing the signed data response. func sign(data: Data, provenance: SigningRequestProvenance) throws -> Data { let reader = OpenSSHReader(data: data) - var hash = reader.readNextChunk() - + let payloadHash = reader.readNextChunk() + let hash: Data // Check if hash is actually an openssh certificate and reconstruct the public key if it is - if let certPublicKey = try? getPublicKeyFromCert(certBlob: hash) { - hash = certPublicKey + if let certificatePublicKey = certificateHandler.publicKeyHash(from: payloadHash) { + hash = certificatePublicKey + } else { + hash = payloadHash } guard let (store, secret) = secret(matching: hash) else { @@ -200,74 +185,6 @@ extension Agent { return signedData } - - /// Reconstructs a public key from a ``Data`` object that contains an OpenSSH certificate. Currently only ecdsa certificates are supported - /// - Parameter certBlock: The openssh certificate to extract the public key from - /// - Returns: A ``Data`` object containing the public key in OpenSSH wire format - func getPublicKeyFromCert(certBlob: Data) throws -> Data { - let reader = OpenSSHReader(data: certBlob) - let certType = String(decoding: reader.readNextChunk(), as: UTF8.self) - - switch certType { - case "ecdsa-sha2-nistp256-cert-v01@openssh.com", - "ecdsa-sha2-nistp384-cert-v01@openssh.com", - "ecdsa-sha2-nistp521-cert-v01@openssh.com": - - _ = reader.readNextChunk() // nonce - let curveIdentifier = reader.readNextChunk() - let publicKey = reader.readNextChunk() - - if let curveType = certType.replacingOccurrences(of: "-cert-v01@openssh.com", with: "").data(using: .utf8) { - return writer.lengthAndData(of: curveType) + - writer.lengthAndData(of: curveIdentifier) + - writer.lengthAndData(of: publicKey) - } else { - throw OpenSSHCertificateError.parsingFailed - } - default: - throw OpenSSHCertificateError.unsupportedType - } - } - - - /// Attempts to find an OpenSSH Certificate that corresponds to a ``Secret`` - /// - Parameter secret: The secret to search for a certificate with - /// - Returns: Two ``Data`` objects containing the certificate and certificate name respectively - func checkForCert(secret: AnySecret) throws -> (Data, Data) { - let minimalHex = writer.openSSHMD5Fingerprint(secret: secret).replacingOccurrences(of: ":", with: "") - let certificatePath = certsPath.appending("/").appending("\(minimalHex)-cert.pub") - - if FileManager.default.fileExists(atPath: certificatePath) { - logger.debug("Found certificate for \(secret.name)") - do { - let certContent = try String(contentsOfFile:certificatePath, encoding: .utf8) - let certElements = certContent.trimmingCharacters(in: .whitespacesAndNewlines).components(separatedBy: " ") - - if certElements.count >= 2 { - if let certDecoded = Data(base64Encoded: certElements[1] as String) { - if certElements.count >= 3 { - if let certName = certElements[2].data(using: .utf8) { - return (certDecoded, certName) - } 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") - return (certDecoded, certName) - } else { - throw OpenSSHCertificateError.parsingFailed - } - } - } else { - logger.warning("Certificate found for \(secret.name) but failed to decode base64 key") - throw OpenSSHCertificateError.parsingFailed - } - } - } catch { - logger.warning("Certificate found for \(secret.name) but failed to load") - throw OpenSSHCertificateError.parsingFailed - } - } - - throw OpenSSHCertificateError.doesNotExist - } } @@ -308,6 +225,7 @@ extension Agent { case unhandledType case noMatchingKey case unsupportedKeyType + case notOpenSSHCertificate } } diff --git a/Sources/Packages/Sources/SecretAgentKit/SSHAgentProtocol.swift b/Sources/Packages/Sources/SecretAgentKit/SSHAgentProtocol.swift index bd3cefb..4c45616 100644 --- a/Sources/Packages/Sources/SecretAgentKit/SSHAgentProtocol.swift +++ b/Sources/Packages/Sources/SecretAgentKit/SSHAgentProtocol.swift @@ -1,11 +1,11 @@ import Foundation -/// A namespace for the SSH Agent Protocol, as described in https://tools.ietf.org/id/draft-miller-ssh-agent-01.html +/// A namespace for the SSH Agent Protocol, as described in https://datatracker.ietf.org/doc/html/draft-miller-ssh-agent#section-5.1 public enum SSHAgent {} extension SSHAgent { - /// The type of the SSH Agent Request, as described in https://tools.ietf.org/id/draft-miller-ssh-agent-01.html#rfc.section.5.1 + /// The type of the SSH Agent Request, as described in https://datatracker.ietf.org/doc/html/draft-miller-ssh-agent#section-5.1 public enum RequestType: UInt8, CustomDebugStringConvertible { case requestIdentities = 11 @@ -21,10 +21,11 @@ extension SSHAgent { } } - /// The type of the SSH Agent Response, as described in https://tools.ietf.org/id/draft-miller-ssh-agent-01.html#rfc.section.5.1 + /// The type of the SSH Agent Response, as described in https://datatracker.ietf.org/doc/html/draft-miller-ssh-agent#section-5.1 public enum ResponseType: UInt8, CustomDebugStringConvertible { case agentFailure = 5 + case agentSuccess = 6 case agentIdentitiesAnswer = 12 case agentSignResponse = 14 @@ -32,6 +33,8 @@ extension SSHAgent { switch self { case .agentFailure: return "AgentFailure" + case .agentSuccess: + return "AgentSuccess" case .agentIdentitiesAnswer: return "AgentIdentitiesAnswer" case .agentSignResponse: diff --git a/Sources/Packages/Sources/SecretKit/OpenSSH/OpenSSHCertificateHandler.swift b/Sources/Packages/Sources/SecretKit/OpenSSH/OpenSSHCertificateHandler.swift new file mode 100644 index 0000000..435c310 --- /dev/null +++ b/Sources/Packages/Sources/SecretKit/OpenSSH/OpenSSHCertificateHandler.swift @@ -0,0 +1,120 @@ +import Foundation +import OSLog + +/// Manages storage and lookup for OpenSSH certificates. +public class OpenSSHCertificateHandler { + + private let publicKeyFileStoreController = PublicKeyFileStoreController(homeDirectory: NSHomeDirectory()) + private let logger = Logger() + private let writer = OpenSSHKeyWriter() + private var keyBlobsAndNames: [AnySecret: (Data, Data)] = [:] + + /// Initializes an OpenSSHCertificateHandler. + public init() { + } + + /// Reloads any certificates in the PublicKeys folder. + /// - Parameter secrets: the secrets to look up corresponding certificates for. + public func reloadCertificates(for secrets: [AnySecret]) { + guard publicKeyFileStoreController.hasAnyCertificates else { + logger.log("No certificates, short circuiting") + return + } + keyBlobsAndNames = secrets.reduce(into: [:]) { partialResult, next in + partialResult[next] = try? loadKeyblobAndName(for: next) + } + } + + /// Whether or not the certificate handler has a certifiicate associated with a given secret. + /// - Parameter secret: The secret to check for a certificate. + /// - Returns: A boolean describing whether or not the certificate handler has a certifiicate associated with a given secret + public func hasCertificate(for secret: SecretType) -> Bool { + keyBlobsAndNames[AnySecret(secret)] != nil + } + + + /// Reconstructs a public key from a ``Data``, if that ``Data`` contains an OpenSSH certificate hash. Currently only ecdsa certificates are supported + /// - Parameter certBlock: The openssh certificate to extract the public key from + /// - Returns: A ``Data`` object containing the public key in OpenSSH wire format if the ``Data`` is an OpenSSH certificate hash, otherwise nil. + public func publicKeyHash(from hash: Data) -> Data? { + let reader = OpenSSHReader(data: hash) + let certType = String(decoding: reader.readNextChunk(), as: UTF8.self) + + switch certType { + case "ecdsa-sha2-nistp256-cert-v01@openssh.com", + "ecdsa-sha2-nistp384-cert-v01@openssh.com", + "ecdsa-sha2-nistp521-cert-v01@openssh.com": + _ = reader.readNextChunk() // nonce + let curveIdentifier = reader.readNextChunk() + let publicKey = reader.readNextChunk() + + let curveType = certType.replacingOccurrences(of: "-cert-v01@openssh.com", with: "").data(using: .utf8)! + return writer.lengthAndData(of: curveType) + + writer.lengthAndData(of: curveIdentifier) + + writer.lengthAndData(of: publicKey) + default: + return nil + } + } + + /// Attempts to find an OpenSSH Certificate that corresponds to a ``Secret`` + /// - Parameter secret: The secret to search for a certificate with + /// - Returns: A (``Data``, ``Data``) tuple containing the certificate and certificate name, respectively. + public func keyBlobAndName(for secret: SecretType) throws -> (Data, Data)? { + keyBlobsAndNames[AnySecret(secret)] + } + + /// Attempts to find an OpenSSH Certificate that corresponds to a ``Secret`` + /// - Parameter secret: The secret to search for a certificate with + /// - Returns: A (``Data``, ``Data``) tuple containing the certificate and certificate name, respectively. + private func loadKeyblobAndName(for secret: SecretType) throws -> (Data, Data)? { + let certificatePath = publicKeyFileStoreController.sshCertificatePath(for: secret) + guard FileManager.default.fileExists(atPath: certificatePath) else { + return nil + } + + logger.debug("Found certificate for \(secret.name)") + let certContent = try String(contentsOfFile:certificatePath, encoding: .utf8) + let certElements = certContent.trimmingCharacters(in: .whitespacesAndNewlines).components(separatedBy: " ") + + guard certElements.count >= 2 else { + logger.warning("Certificate found for \(secret.name) but failed to load") + throw OpenSSHCertificateError.parsingFailed + } + guard let certDecoded = Data(base64Encoded: certElements[1] as String) else { + logger.warning("Certificate found for \(secret.name) but failed to decode base64 key") + throw OpenSSHCertificateError.parsingFailed + } + + if certElements.count >= 3, let certName = certElements[2].data(using: .utf8) { + return (certDecoded, certName) + } 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") + return (certDecoded, certName) + } else { + throw OpenSSHCertificateError.parsingFailed + } + } + +} + +extension OpenSSHCertificateHandler { + + enum OpenSSHCertificateError: LocalizedError { + case unsupportedType + case parsingFailed + case doesNotExist + + public var errorDescription: String? { + switch self { + case .unsupportedType: + return "The key type was unsupported" + case .parsingFailed: + return "Failed to properly parse the SSH certificate" + case .doesNotExist: + return "Certificate does not exist" + } + } + } + +} diff --git a/Sources/Packages/Sources/SecretKit/PublicKeyStandinFileController.swift b/Sources/Packages/Sources/SecretKit/PublicKeyStandinFileController.swift index cbf40a6..e30627a 100644 --- a/Sources/Packages/Sources/SecretKit/PublicKeyStandinFileController.swift +++ b/Sources/Packages/Sources/SecretKit/PublicKeyStandinFileController.swift @@ -45,6 +45,18 @@ public class PublicKeyFileStoreController { return directory.appending("/").appending("\(minimalHex).pub") } + /// Short-circuit check to ship enumerating a bunch of paths if there's nothing in the cert directory. + public var hasAnyCertificates: Bool { + do { + return try FileManager.default + .contentsOfDirectory(atPath: directory) + .filter { $0.hasSuffix("-cert.pub") } + .isEmpty == false + } catch { + return false + } + } + /// The path for a Secret's SSH Certificate public key. /// - Parameter secret: The Secret to return the path for. /// - Returns: The path to the SSH Certificate public key. diff --git a/Sources/Packages/Sources/SecretKit/SecretStoreList.swift b/Sources/Packages/Sources/SecretKit/SecretStoreList.swift index 57fed98..fbde248 100644 --- a/Sources/Packages/Sources/SecretKit/SecretStoreList.swift +++ b/Sources/Packages/Sources/SecretKit/SecretStoreList.swift @@ -31,6 +31,10 @@ public class SecretStoreList: ObservableObject { stores.reduce(false, { $0 || $1.isAvailable }) } + public var allSecrets: [AnySecret] { + stores.flatMap(\.secrets) + } + } extension SecretStoreList { diff --git a/Sources/SecretAgent/AppDelegate.swift b/Sources/SecretAgent/AppDelegate.swift index 45f176c..e21587f 100644 --- a/Sources/SecretAgent/AppDelegate.swift +++ b/Sources/SecretAgent/AppDelegate.swift @@ -34,9 +34,9 @@ class AppDelegate: NSObject, NSApplicationDelegate { self.socketController.handler = self.agent.handle(reader:writer:) } NotificationCenter.default.addObserver(forName: .secretStoreReloaded, object: nil, queue: .main) { [self] _ in - try? publicKeyFileStoreController.generatePublicKeys(for: storeList.stores.flatMap({ $0.secrets }), clear: true) + try? publicKeyFileStoreController.generatePublicKeys(for: storeList.allSecrets, clear: true) } - try? publicKeyFileStoreController.generatePublicKeys(for: storeList.stores.flatMap({ $0.secrets }), clear: true) + try? publicKeyFileStoreController.generatePublicKeys(for: storeList.allSecrets, clear: true) notifier.prompt() updateSink = updater.$update.sink { update in guard let update = update else { return }