Initial openssh certificate support #416

unreality · 2022-10-26T06:56:41Z

This change adds initial OpenSSH certificate support and closes #269

If a a cert file exists for a public key in the 'PublicKeys' directory, SecretAgent will use it instead of the PublicKey.

For example if Secretive creates a key with the path ~/Library/Containers/com.maxgoedjen.Secretive.SecretAgent/Data/PublicKeys/266b0718b08bc8653c885ae41534e1c0.pub it will check if ~/Library/Containers/com.maxgoedjen.Secretive.SecretAgent/Data/PublicKeys/266b0718b08bc8653c885ae41534e1c0-cert.pub exists, and return the contents as an available identity.

Upon a signing request it will attempt to check if the signing request is actually for a certificate, resolve it and sign using the correct secret instead.

This is probably pretty rough, im not a Swift dev normally, i just really wanted this feature in :)

Please let me know what changes need to occur to get this merged in.

maxgoedjen

Damn, this is looking awesome, thanks for doing this! I want to take some time to review this properly and make sure I understand everything that's going on here, but gimme a couple days and we can get this merged. Exciting! 🥳

maxgoedjen · 2022-10-26T07:47:39Z

This is probably pretty rough, im not a Swift dev normally, i just really wanted this feature in :)

Love to see it, welcome to the fold. I'll probably have a few minor "this is a swiftier way to do it" type things but it looks pretty solid for someone newish to the language!

maxgoedjen · 2022-10-26T07:50:41Z

Sources/Packages/Sources/SecretAgentKit/Agent.swift

@@ -101,7 +135,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)
-        let hash = reader.readNextChunk()
+        var hash = reader.readNextChunk()


nit: you can avoid having this be a var by doing something like this:

let myInstance: MyType if condition { myInstance = someFuncX() } else { myInstance = someFuncY() }

Basically it can still be a let so long as it's 1) only written once and 2) set before read.

maxgoedjen · 2022-10-26T08:00:31Z

Sources/Packages/Sources/SecretAgentKit/Agent.swift

@@ -83,12 +85,44 @@ extension Agent {
        var count = UInt32(secrets.count).bigEndian
        let countData = Data(bytes: &count, count: UInt32.bitWidth/8)
        var keyData = Data()
-        let writer = OpenSSHKeyWriter()


maxgoedjen · 2022-10-26T08:01:33Z

Sources/Packages/Sources/SecretAgentKit/Agent.swift

        for secret in secrets {
-            let keyBlob = writer.data(secret: secret)
+            let minimalHex = writer.openSSHMD5Fingerprint(secret: secret).replacingOccurrences(of: ":", with: "")
+            let certificatePath = certsPath.appending("/").appending("\(minimalHex)-cert.pub")


This is one of those areas where I need to defer to someone with experience using the thing (ie, you): is this a reasonable ask for people? To look up what the fingerprint will be for the key, and to put their public key in that location?

There's also some code in the agent that cleans out the public keys folder periodically (to make sure any keys the user deleted don't have a public key hanging out there still).

We might be better off having that be in some sibling path: ie, if my public key is
/Users/max/Library/Containers/com.maxgoedjen.Secretive.SecretAgent/Data/PublicKeys/e1da7c30f1fe9bebb924fbf62fb443b5.pub
we could request the user place their key in
/Users/max/Library/Containers/com.maxgoedjen.Secretive.SecretAgent/Data/SSHCertificates/e1da7c30f1fe9bebb924fbf62fb443b5.pub

The reason i went with this path is that generally the openssh tooling will write the certificate out in that path by default, ie if you pass it in id_ecdsa.pub it generates id_ecdsa-cert.pub so i went with that since the public key path is easy to copy/paste from the interface...

A sibling path would be fine as well though

Precisely why I asked, didn't realize the tool spit it out there, makes perfect sense. I'll take a look at the cleanup code and make sure it doesn't incorrectly cleanup up any cert files.

maxgoedjen · 2022-10-26T08:04:25Z

Sources/Packages/Sources/SecretAgentKit/Agent.swift

+            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) {
+                            certsMap[certDecoded] = keyBlob // Probably a better way to store the certificate/key association than this...
+                            keyBlob = certDecoded
+
+                            if certElements.count >= 3 {
+                                if let certName = certElements[2].data(using: .utf8) {
+                                    curveData = certName
+                                } else {
+                                    Logger().info("Certificate for \(secret.name) does not have a name tag, using curveData instead")
+                                }
+                            }
+                        } else {
+                            Logger().warning("Certificate found for \(secret.name) but failed to decode base64 key, using public key instead")
+                        }
+                    }
+                } catch {
+                    Logger().warning("Certificate found for \(secret.name) but failed to load, using public key instead")
+                }
+            }


Might be cleaner to chunk this all out into a separate function.

Moved this into its own function now

maxgoedjen · 2022-10-26T08:07:14Z

Sources/Packages/Sources/SecretAgentKit/Agent.swift

+                    let certContent = try String(contentsOfFile:certificatePath, encoding: .utf8)
+                    let certElements = certContent.trimmingCharacters(in: .whitespacesAndNewlines).components(separatedBy: " ")
+
+                    if certElements.count >= 2 {


If you define an error type outside the function

public struct SSHCertificateError: Error { }

You can switch to something like this:

guard certElements.count >= 2 else { Logger().warning("Certificate found for \(secret.name) but failed to decode base64 key, using public key instead") throw SSHCertificateError() }

(and likewise anywhere else you have a terminating if like that.

maxgoedjen · 2022-10-26T08:08:21Z

Sources/Packages/Sources/SecretAgentKit/Agent.swift

+
+                    if certElements.count >= 2 {
+                        if let certDecoded = Data(base64Encoded: certElements[1] as String) {
+                            certsMap[certDecoded] = keyBlob // Probably a better way to store the certificate/key association than this...


I might move this to be loaded up at the same point we load in the keys/etc, but this is good for now, I can take care of that later :)

ive removed certsMap and now reconstruct the public key from the cert when a signing request comes in, should be a bit neater

…map in memory

unreality · 2022-10-26T13:11:12Z

@maxgoedjen I've committed most of the requested changes, let me know if anything else needs changing :)

maxgoedjen · 2022-10-27T05:20:54Z

All merged. Thanks again, definitely put it down in the "probably wouldn't have happened without your direct efforts" column :)

There should be a nightly build with this up soon under (you can grab from the Actions tab), I'll probably plan on cutting a new release with this once I wrap up #370.

initial openssh certificate support

10fbc0f

maxgoedjen reviewed Oct 26, 2022

View reviewed changes

Raal Goff added 3 commits October 26, 2022 19:19

use the certificate to construct the public key instead of storing a …

2cce241

…map in memory

move certificate check into its own function and neaten up a few things

4adeabc

final requested changes

fb7c0ce

maxgoedjen approved these changes Oct 27, 2022

View reviewed changes

Merge branch 'main' into openssh-certificate-support

4db7e95

maxgoedjen enabled auto-merge (squash) October 27, 2022 05:17

maxgoedjen merged commit fa0e81c into maxgoedjen:main Oct 27, 2022

This was referenced Oct 27, 2022

Don't delete public cert keys corresponding to Secretive-tracked keys #417

Merged

SSH Certificate Cleanup/Followup #418

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial openssh certificate support #416

Initial openssh certificate support #416

unreality commented Oct 26, 2022

maxgoedjen left a comment

maxgoedjen commented Oct 26, 2022

maxgoedjen Oct 26, 2022

maxgoedjen Oct 26, 2022

maxgoedjen Oct 26, 2022

maxgoedjen Oct 26, 2022

unreality Oct 26, 2022

maxgoedjen Oct 26, 2022

maxgoedjen Oct 26, 2022

unreality Oct 26, 2022

maxgoedjen Oct 26, 2022

maxgoedjen Oct 26, 2022

unreality Oct 26, 2022 •

edited

Loading

unreality commented Oct 26, 2022

maxgoedjen commented Oct 27, 2022

Initial openssh certificate support #416

Initial openssh certificate support #416

Conversation

unreality commented Oct 26, 2022

maxgoedjen left a comment

Choose a reason for hiding this comment

maxgoedjen commented Oct 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unreality Oct 26, 2022 • edited Loading

Choose a reason for hiding this comment

unreality commented Oct 26, 2022

maxgoedjen commented Oct 27, 2022

unreality Oct 26, 2022 •

edited

Loading