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/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) {
+            if let (certificateData, name) = try? certificateHandler.keyBlobAndName(for: secret) {
-                keyBlob = certBlob
+                keyBlob = certificateData
-                curveData = certName
+                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) {
+        if let certificatePublicKey = certificateHandler.publicKeyHash(from: payloadHash) {
-            hash = certPublicKey
+            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
     }
 
 }

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:

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<SecretType: Secret>(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<SecretType: Secret>(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<SecretType: Secret>(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"
+            }
+        }
+    }
+
+}

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.

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 {

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 }