Added SSH Key comment field and fixed build for other users #190

lavalleeale · 2020-12-25T09:13:24Z

No description provided.

Signed-off-by: Alex Lavallee <[email protected]>

maxgoedjen · 2020-12-27T23:35:12Z

Secretive/Controllers/AgentStatusChecker.swift

@@ -9,6 +9,7 @@ protocol AgentStatusCheckerProtocol: ObservableObject {
 class AgentStatusChecker: ObservableObject, AgentStatusCheckerProtocol {

    @Published var running: Bool = false
+    let bundleID = Bundle.main.bundleIdentifier!.replacingOccurrences(of: "Host", with: "SecretAgent")


Overall big 👍 these changes.
This doesn't really need to be a property on the class though I don't think, you can just move this into the secretAgentProcesses block.

Bundle.main.bundleIdentifier!.replacingOccurrences(of: "Host", with: "SecretAgent") this bit is repeated a couple times – might make sense to have a Bundle extension in SecretKit with like hostBundleID and agentBundleID properties.

maxgoedjen · 2020-12-27T23:35:20Z

Secretive/Controllers/LaunchAgentController.swift

@@ -4,7 +4,8 @@ import AppKit
 import OSLog

 struct LaunchAgentController {
-
+
+    let bundleID = Bundle.main.bundleIdentifier!.replacingOccurrences(of: "Host", with: "SecretAgent")


maxgoedjen

Hey @lavalleeale – thanks for the PR, sorry it took me so long to review.

Overall got some good changes – I think there might be a better storage strategy possible that we should experiment with though.

maxgoedjen · 2020-12-27T23:37:41Z

Secretive/Views/CreateSecretView.swift

    @State private var name = ""
    @State private var requiresAuthentication = true
+    let defaults = UserDefaults.standard


I'm not crazy about having this info stored in UserDefaults/View tbh, particularly for the SEP keys.

My inclination would be to add a kSecAttrComment at the key creation

secretive/SecretKit/SecureEnclave/SecureEnclaveStore.swift

Line 46 in 6c3748b

let attributes = [

and read that out into a new property on Secret

(disclaimer: I haven't personally tested this yet so it might run into some issues, the SEP can be a bit weird that way sometimes)

We'll probably also need to figure out an approach for SmartCards.

maxgoedjen · 2020-12-27T23:40:44Z

Secretive/Controllers/AgentStatusChecker.swift

@@ -9,6 +9,7 @@ protocol AgentStatusCheckerProtocol: ObservableObject {
 class AgentStatusChecker: ObservableObject, AgentStatusCheckerProtocol {

    @Published var running: Bool = false
+    let bundleID = Bundle.main.bundleIdentifier!.replacingOccurrences(of: "Host", with: "SecretAgent")


Bundle.main.bundleIdentifier!.replacingOccurrences(of: "Host", with: "SecretAgent") this bit is repeated a couple times – might make sense to have a Bundle extension in SecretKit with like hostBundleID and agentBundleID properties.

maxgoedjen · 2020-12-27T23:43:16Z

Secretive/Views/CreateSecretView.swift


    func save() {
        try! store.create(name: name, requiresAuthentication: requiresAuthentication)
+        if comment != "" {
+            defaults.set(comment, forKey: name)


Having the comment be the default is nice 👍
(when we switch to the comment being on the keychain entry, I think it probably makes sense to just to do this at view time, and not bake it in to the stored secret. This will make it nicer if the user changes their username, or has secrets created before this).

vladimyr · 2020-12-27T23:47:14Z

@maxgoedjen I believe you tagged the wrong guy 😉

maxgoedjen · 2020-12-27T23:51:33Z

@vladimyr betrayed by autocomplete!

…eID properties

lavalleeale · 2020-12-28T01:19:35Z

Hey, maybe I just read wrong, but I was under the impression that kSecAttrComment could not be changed after it was set. Obviously this PR doesn't let you change the comment later, but that might be a good idea down the road.

lavalleeale

Hey, I changed it to being an extension

maxgoedjen · 2020-12-28T02:45:02Z

Hey, maybe I just read wrong, but I was under the impression that kSecAttrComment could not be changed after it was set. Obviously this PR doesn't let you change the comment later, but that might be a good idea down the road.

I'm not sure about that tbh, you could be right there. I'm not terribly concerned with that if it is the case tho, since keys are basically immutable as is.

maxgoedjen · 2020-12-28T02:45:32Z

@lavalleeale looks like unit tests failed to build

1366
Test session results, code coverage, and logs:
1367
	Value of type 'Bundle' has no member 'agentBundleID'
1368
	/Users/runner/Library/Developer/Xcode/DerivedData/Secretive-acdauzrjufafyfgbbuwlkimwgdpy/Logs/Test/Test-Secretive-2020.12.28_02-41-23-+0000.xcresult
1369
	Value of type 'Bundle' has no member 'agentBundleID'
1370

1371
	Value of type 'Bundle' has no member 'agentBundleID'
1372
	Testing cancelled because the build failed.
1373

maxgoedjen · 2020-12-28T02:46:46Z

SecretKit/Common/bundleIDs.swift

+//
+//  bundleIDs.swift
+//  SecretKit
+//
+//  Created by Alex lavallee on 12/27/20.
+//  Copyright © 2020 Max Goedjen. All rights reserved.
+//
+


Suggested change

//

// bundleIDs.swift

// SecretKit

//

// Created by Alex lavallee on 12/27/20.

// Copyright © 2020 Max Goedjen. All rights reserved.

//

lavalleeale · 2020-12-28T03:06:58Z

Sorry about that, TIL that Xcode stores a reference of all the files in the project in the same file where it stores signing data

maxgoedjen · 2020-12-28T06:50:23Z

All good, I miss it about half the time myself ;)

lavalleeale · 2020-12-28T07:02:55Z

So is there anything else in the PR that you want me to fix?

maxgoedjen · 2021-01-18T02:04:27Z

Hey @lavalleeale – sorry to be a bummer about this but I'm still pretty uncomfortable about storing metadata like this outside of the SEP. If we can find a way to remedy that I'd be receptive to a PR along those lines. (I'd also still gratefully merge the changes you made to the project to simplify signing/bundle IDs, if you'd be willing to split that out into a separate PR).

maxgoedjen · 2021-01-18T03:32:59Z

I sketched out a few ideas we might be able to use to address this in #201 – it's probably a nontrivial amount of work so don't feel obligated to take it on yourself, I'll take a look at implementing for probably ~2.2.0 or so.

lavalleeale · 2021-01-18T05:46:12Z

I'll take a look at the metadata idea, but comments are a part of the public key, which should be fine if it is public, right?

maxgoedjen · 2021-01-18T06:08:15Z

comments are a part of the public key, which should be fine if it is public, right?

To an extent. Like if you were looking at my keys on https://github.com/maxgoedjen.keys it's fine that you see the comments there. Having the comments be in a non-locked-down area like user defaults can potentially leak what services you're using (ie, another process could see that I use GitHub, that I have another Mac named XYZ, etc) which IMO somewhat violates the security expectations people have of the app.

lavalleeale · 2021-01-18T06:26:00Z

I'm just saying that the last field in the public key is the comment, maybe specify it is part of the public key?

lavalleeale added 5 commits December 25, 2020 01:12

Changed SSH key comment

3eaf155

Signed-off-by: Alex Lavallee <[email protected]>

Added comment option when generating key

e6e2902

Allowed setup to properly show for all builds

f456465

Fixed rest of app for non-standard build

9c4b4ac

Allows auto-launch for every user's build

670bfb1

maxgoedjen reviewed Dec 27, 2020

View reviewed changes

maxgoedjen requested changes Dec 27, 2020

View reviewed changes

maxgoedjen mentioned this pull request Dec 27, 2020

Overzealous error when in the user Applications folder #177

Closed

made a Bundle extension in SecretKit with hostBundleID and agentBundl…

edeb603

…eID properties

lavalleeale commented Dec 28, 2020

View reviewed changes

maxgoedjen reviewed Dec 28, 2020

View reviewed changes

lavalleeale added 2 commits December 27, 2020 19:03

Fixed project.pbxproj

33bad0a

Removed default Xcode comment

cbb22b2

maxgoedjen mentioned this pull request Jan 18, 2021

Add keychain-backed "metadata" store #201

Open

Merge branch 'main' into main

607cd1a

Merge branch 'main' into main

9bd8e97

Merge branch 'main' into main

ab27f20

lavalleeale closed this Jan 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added SSH Key comment field and fixed build for other users #190

Added SSH Key comment field and fixed build for other users #190

lavalleeale commented Dec 25, 2020

maxgoedjen Dec 27, 2020

maxgoedjen Dec 27, 2020

maxgoedjen Dec 27, 2020

maxgoedjen left a comment •

edited

Loading

maxgoedjen Dec 27, 2020

maxgoedjen Dec 27, 2020

maxgoedjen Dec 27, 2020

maxgoedjen Dec 27, 2020

vladimyr commented Dec 27, 2020

maxgoedjen commented Dec 27, 2020

lavalleeale commented Dec 28, 2020

lavalleeale left a comment •

edited

Loading

maxgoedjen commented Dec 28, 2020

maxgoedjen commented Dec 28, 2020

maxgoedjen Dec 28, 2020

lavalleeale commented Dec 28, 2020

maxgoedjen commented Dec 28, 2020

lavalleeale commented Dec 28, 2020

maxgoedjen commented Jan 18, 2021

maxgoedjen commented Jan 18, 2021

lavalleeale commented Jan 18, 2021

maxgoedjen commented Jan 18, 2021

lavalleeale commented Jan 18, 2021

Added SSH Key comment field and fixed build for other users #190

Added SSH Key comment field and fixed build for other users #190

Conversation

lavalleeale commented Dec 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxgoedjen left a comment • edited Loading

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

vladimyr commented Dec 27, 2020

maxgoedjen commented Dec 27, 2020

lavalleeale commented Dec 28, 2020

lavalleeale left a comment • edited Loading

Choose a reason for hiding this comment

maxgoedjen commented Dec 28, 2020

maxgoedjen commented Dec 28, 2020

Choose a reason for hiding this comment

lavalleeale commented Dec 28, 2020

maxgoedjen commented Dec 28, 2020

lavalleeale commented Dec 28, 2020

maxgoedjen commented Jan 18, 2021

maxgoedjen commented Jan 18, 2021

lavalleeale commented Jan 18, 2021

maxgoedjen commented Jan 18, 2021

lavalleeale commented Jan 18, 2021

maxgoedjen left a comment •

edited

Loading

lavalleeale left a comment •

edited

Loading