SSH Certificate Cleanup/Followup #418
Conversation
| func getPublicKeyFromCert(certBlob: Data) throws -> Data { | ||
| let reader = OpenSSHReader(data: certBlob) | ||
| /// - Returns: A ``Data`` object containing the public key in OpenSSH wire format if the ``Data`` is an OpenSSH certificate hash, otherwise nil. | ||
| func publicKeyHashFromSSHCertificateHash(_ hash: Data) -> Data? { |
There was a problem hiding this comment.
Ended up thinking about this a bit more and feeling non-throwing is the right choice here, since many of these failures aren't failures exactly, so much as "this object we are trying to parse that maybe is or isn't a cert hash ended up not being that"
| if certElements.count >= 3 { | ||
| if let certName = certElements[2].data(using: .utf8) { | ||
| return (certDecoded, certName) | ||
| } else if let certName = secret.name.data(using: .utf8) { |
There was a problem hiding this comment.
@unreality was this else maybe supposed to correspond to the if on line 245? I don't see that case handled here.
There was a problem hiding this comment.
@maxgoedjen Its an attempt to handle if the certName in the certificate file is not utf8 encodable, so it will use the secret.name instead in the hopes that secret.name is utf8 encodable instead.
You are right that if there is only 2 certElements, it falls through and fails - perhaps use secret.name if that is the case
|
@unreality I split off some of this into a dedicated controller that loads the public keys on agent startup – read-on-identities-request had me a little concerned with doing disk reads every request. Mind taking this out for a spin when you get a chance and confirming I didn't break anything? ;) |
|
@maxgoedjen Sure i'll check it out.. Just on a brief look at the code though - what happens if someone adds a certificate after SecretAgent is running? How does SecretAgent determine that there is now a certificate there? On my quick look through it doesnt seem to do any monitoring of the directory, or reload the certificate map at any time except launch? |
|
Yeah, that's kinda the problem in my mind too. As of this draft, right now the answer is "it doesn't," which isn't great. Short of doing a much of file system activity every auth, I don't know what a better solution is. Maybe invalidate on identities call if the hash is a valid certificate one? |
|
Hm that hash is on sign not lookup, never mind (could still fault it there, but "fail once to work next time" is kinda hacky. |
|
I suppose the 'proper' way would be perhaps to add That way |
|
I like that line of thinking. Does the certificate generation flow make that call currently, or instruct users to do that? |
|
It depends on their flow, but adding the certificate using Some tooling already does this step (or attempts to) so it should mesh well with user flows.. |
I think im wrong about this being a possible solution, it seems See https://bugzilla.mindrot.org/show_bug.cgi?id=3212 for the open feature request |
|
@unreality there is a |
|
@maxgoedjen perhaps a file watcher could be set up to monitor the directory and mark the cache as dirty to re-poll? eg https://github.com/robovm/apple-ios-samples/blob/master/ListerforwatchOSiOSandOSX/Swift/ListerKit/DirectoryMonitor.swift (i havent tested this, just doing some looking around for solutions at the moment) |
|
@unreality ok think this is finally good to go – I've got this re-checking for certs in the identity codepath (if there aren't any, I short circuit it, should be OK perf-wise). Mind giving this one more test just to make sure I didn't break anything? I'll merge this soon + cut a new release after that's done. |
|
Gonna go ahead and land this, I'll let it bake in the nightly channel for a couple more days – please @ me if you notice anything weird. |
|
I was just testing this as you merged it :) But it all looks fine, thanks for merging it in |
|
@unreality good timing! :) I'll still probably let it bake for a day or two (I want to fix #430 before release) but I think I'll cut this release sometime this week. Just want to give one more huge thank you for driving this feature – this is the first big community-contributed feature in Secretive, and I'm really excited for it to land! |
Followup to #416