[RFC] Keychain-backed settings #536

paulhdk · 2024-02-29T19:39:30Z

As promised in #524 , here's my current progress in the form of an RFC pull request. I was hoping to get some feedback and make sure that I'm running in the right direction 🙂

This pull request adds:

An API for storing to and reading from the keychain via simple get and set functions.
Replace the use of UserDefaults in JustUpdatedChecker.swift.
A settings screen which can be accessed via File -> Settings... or CMD + ,.
The ability to disable the SSH comment in the new settings screen by selecting "None".

Here's how the Settings looks:

Concrete questions I have:

What styles should I add for the SSH comments?
How should the different SSH comment styles be presented? I guess it would make sense to add some kind of preview.
What else belongs in the settings screen?
Did I understand the keychain suggestion correctly? Should it be used somewhere, or is the API enough for now?
Should the settings have an "About" or "Acknowledgements" tab?
Re: how UserDefaults are replaced inJustUpdatedChecker.swift, is this how it's supposed to work? There is a slight reinterpretation of the keychain functionality happening here since the keychain thinks that we're storing a password, which strictly speaking isn't the case 🙂

maxgoedjen

Hey @PBHDK – made a first review pass here. I think your fundamental approach here is solid, but currently the stuff that needs to be in secure store is in defaults, and the stuff in defaults needs to be in secure store ;)

Sources/Secretive/Helpers/SettingsHelper.swift

maxgoedjen · 2024-03-01T03:51:11Z

Sources/Secretive/Views/SecretDetailView.swift

@@ -4,6 +4,7 @@ import SecretKit
 struct SecretDetailView<SecretType: Secret>: View {

    @State var secret: SecretType
+    @AppStorage("com.maxgoedjen.Secretive.commentStyle") var style: CommentStyle = .keyAndHost


This is just going to be pulling directly from UserDefaults, we want this plugged into the SettingsStore instead.

maxgoedjen · 2024-03-01T03:51:59Z

Sources/Secretive/Controllers/JustUpdatedChecker.swift

-        let lastBuild = UserDefaults.standard.object(forKey: Constants.previousVersionUserDefaultsKey) as? String ?? "None"
+        let lastBuild = SettingsStore.get(key: Constants.previousVersionUserDefaultsKey) ?? "None"
        let currentBuild = Bundle.main.infoDictionary!["CFBundleShortVersionString"] as! String
-        UserDefaults.standard.set(currentBuild, forKey: Constants.previousVersionUserDefaultsKey)


Let's keep these in UserDefaults as-is for now, these don't need to be in secure storage.

maxgoedjen · 2024-03-01T23:05:02Z

Sources/Secretive/Views/SecretDetailView.swift

+        switch style {
+        case CommentStyle.none:
+            keyWriter.openSSHString(secret: secret, comment: "")
+        default:
+            keyWriter.openSSHString(secret: secret, comment: "\(dashedKeyName)@\(dashedHostName)")
+        }


Suggested change

switch style {

case CommentStyle.none:

keyWriter.openSSHString(secret: secret, comment: "")

default:

keyWriter.openSSHString(secret: secret, comment: "\(dashedKeyName)@\(dashedHostName)")

}

switch style {

case .none:

keyWriter.openSSHString(secret: secret, comment: "")

case .keyAndHost:

keyWriter.openSSHString(secret: secret, comment: "\(dashedKeyName)@\(dashedHostName)")

}

maxgoedjen · 2024-03-01T23:06:24Z

Sources/Secretive/Views/SecretDetailView.swift

@@ -42,9 +43,13 @@ struct SecretDetailView<SecretType: Secret>: View {
    }

    var keyString: String {
-        keyWriter.openSSHString(secret: secret, comment: "\(dashedKeyName)@\(dashedHostName)")
+        switch style {


As this method gets more complicated, it MIGHT make sense to move it into OpenSSHKeyWriter – just instead of taking a comment: String parameter, we define that enum inside OpenSSHKeyWriter and pass an enum value commentStyle: CommentStyle

dashedKeyName and dashedHostName are both defined in SecretDetailView.swift. That means we would also have to move those into OpenSSHKeyWriter.swift too, yes?

EDIT: forgot to tag you, @maxgoedjen 🙂

@maxgoedjen just giving this a polite bump in case it got lost 🙂

Yeah they're just there since I think that's the only place that used them til now - feel free to move them somewhere more appropriate

maxgoedjen · 2024-03-01T23:07:43Z

Sources/Secretive/Views/SettingsView.swift

+//
+//  SettingsView.swift
+//  Secretive
+//
+//  Created by Paul Heidekrüger on 05.02.24.
+//  Copyright © 2024 Max Goedjen. All rights reserved.
+//
+


Suggested change

//

// SettingsView.swift

// Secretive

//

// Created by Paul Heidekrüger on 05.02.24.

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

//

maxgoedjen · 2024-03-01T23:07:55Z

Sources/Secretive/Helpers/SettingsHelper.swift

+//
+//  SettingsHelper.swift
+//  Secretive
+//
+//  Created by Paul Heidekrüger on 27.02.24.
+//  Copyright © 2024 Max Goedjen. All rights reserved.
+//
+


Suggested change

//

// SettingsHelper.swift

// Secretive

//

// Created by Paul Heidekrüger on 27.02.24.

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

//

maxgoedjen · 2024-03-01T23:08:33Z

Sources/Secretive/Helpers/SettingsHelper.swift

+}
+
+extension SettingsStore {
+    static func set(key: String, value: String) -> Bool {


Probably makes sense to have the core get/set methods her expose Data directly (and maybe have a couple thin wrapper methods for string).

maxgoedjen · 2024-03-01T23:09:47Z

Sources/Secretive/Helpers/SettingsHelper.swift

+import Foundation
+
+class SettingsStore {
+    static let service = "com.maxgoedjen.Secretive"


nit: move this into an enum Constants like this: https://github.com/maxgoedjen/secretive/blob/main/Sources/Packages/Sources/SecureEnclaveSecretKit/SecureEnclaveStore.swift#L304-L312

maxgoedjen · 2024-03-01T23:11:07Z

Sources/Secretive/Helpers/SettingsHelper.swift

+                                           kSecAttrAccount as String: key,
+                                           kSecAttrServer as String: service,
+                                           kSecValueData as String: valueData]
+            // FIXME: Make this non-blocking as described here: https://developer.apple.com/documentation/security/1401659-secitemadd 


The only way you'll really get around this is by caching these values. I'm not super worried about this in this context, assuming you're not seeing any noticeable chug from it.

maxgoedjen · 2024-03-01T23:18:05Z

Answering your questions from PR:

What styles should I add for the SSH comments?
I put a few in comments, but I think a few variations of user@host, [email protected], secretive@host, etc probably make sense. If we can figure out a way to make it an actual format string with a few components the user can rearrange, I'd love that, but may be a little out of scope.
How should the different SSH comment styles be presented? I guess it would make sense to add some kind of preview.
A preview is a great idea. Having a CopyableView below the editor with a dummy secret (or one from their actual store) and what the comment would look like would be cool.
What else belongs in the settings screen?
Mainly we just want the comment style there for now. If/when I can figure out a CLI interface, enabling it will also happen there.
Did I understand the keychain suggestion correctly? Should it be used somewhere, or is the API enough for now?
Kinda – commented in the code, but I think it's basically just backwards now. Comment style goes in keychain, update checker sticks with user defaults.
Should the settings have an "About" or "Acknowledgements" tab?
macOS convention is to have that be a different thing, it's already there in Secretive->About Secretive, so let's keep that there for now.
Re: how UserDefaults are replaced inJustUpdatedChecker.swift, is this how it's supposed to work? There is a slight reinterpretation of the keychain functionality happening here since the keychain thinks that we're storing a password, which strictly speaking isn't the case 🙂
Commented on that there, but basically we don't want to be using this for updated checker. That being said, GenericPassword is a verrrrrry broad keychain category, it's okay to store whatever in there. Doesn't have to be a literal password.

paulhdk · 2024-03-04T11:32:39Z

Thanks so much for the detailed comments, @maxgoedjen !

I'll try to address them ASAP, but it might end up being next week since I have to prioritise some other work in the next few days.

subscripts and make settingsStore an environmentObject Fixes: maxgoedjen#536 (comment)

Fixes: maxgoedjen#536 (comment)

subscripts and make settingsStore an environmentObject Fixes: maxgoedjen#536 (comment)

Fixes: maxgoedjen#536 (comment)

ChristopherA · 2024-04-27T22:13:24Z

We too have been puzzling on a number of best practices for SSH keys. In particular, we believe key separation not only for each computer, but separation of keys "key usage/proof purpose" is critical, and thus you should NOT use the same key for auth & signing. This doesn't seem like something Secretive can support right now.

My document is still an early draft, but will give you some of our thinking on naming conventions for ssh key files.

https://gist.github.com/ChristopherA/3d6a2f39c4b623a1a287b3fb7e0aa05b

Regarding key comments, GitHub doesn't seem to preserve them when uploaded, however, I do try to make the "title" field in GitHub match the SSH comment. You can see my own GitHub signing keys at https://api.github.com/users/ChristopherA/ssh_signing_keys along with their titles/comments. They are not completely consistent yet as I'm still working on our conventions to support best practices there.

(If you are interested, this will give you a clue where we hope to go with ssh signatures in the more long-term https://gist.github.com/ChristopherA/dcf963c3849bc0d67b35c215efd15f86 & https://github.com/BlockchainCommons/ssh-envelope-python )

SSH signing and improving digital attribution and provenance is an important project for us this quarter, so welcome any advice.

maxgoedjen · 2024-04-27T22:16:58Z

you should NOT use the same key for auth & signing. This doesn't seem like something Secretive can support right now.

That's (assuming I'm understanding you correctly) definitely not the case. I personally have a different key for SSH access and signing git commits (which you can see in my profile). Is there something about this flow that doesn't work for you? Essentially I just have one key that I tell git to use for signing commits, and another one for SSH access.

ChristopherA · 2024-04-27T22:26:42Z

That's (assuming I'm understanding you correctly) definitely not the case. I personally have a different key for SSH access and signing git commits (which you can see in my profile). Is there something about this flow that doesn't work for you? Essentially I just have one key that I tell git to use for signing commits, and another one for SSH access.

Yes, that is what is required. I think the docs for configuring this are not clear. When searching for an answer, I saw somewhere (in an issues or PR?) that you needed to put the same public key in both the auth and signing.

ChristopherA · 2024-04-27T22:28:09Z

I found the errant doc:

https://github.com/maxgoedjen/secretive/pull/439/files

Add it to your GitHub account as an Auth and a Signing key, i.e. add it twice.

maxgoedjen · 2024-04-27T22:29:45Z

Yeah that's not merged ;) – there's no requirement on Secretive's side for that to be the case at all, it's totally agnostic of key usage. Some people may find that to be the most convenient setup, but there's no requirement to that effect.

ChristopherA · 2024-04-27T22:32:35Z

Thanks. We are still experimenting with various proof-of-concepts to support best practices, but when we are satisfied, I'll consider a improved PR for docs here.

paulhdk · 2024-07-25T07:32:56Z

@maxgoedjen sorry for letting this PR sit so long. Aiming to finally address all of your feedback within the week!

subscripts and make settingsStore an environmentObject Fixes: maxgoedjen#536 (comment)

Fixes: maxgoedjen#536 (comment)

subscripts and make settingsStore an environmentObject Fixes: maxgoedjen#536 (comment)

Fixes: maxgoedjen#536 (comment)

subscripts and make settingsStore an environmentObject Fixes: maxgoedjen#536 (comment)

Fixes: maxgoedjen#536 (comment)

…ift" This reverts commit ae8a21a.

maxgoedjen requested changes Mar 1, 2024

View reviewed changes

paulhdk added a commit to paulhdk/secretive that referenced this pull request Apr 5, 2024

Make SettingsStore functions non-static, re-implement them as

80b180e

subscripts and make settingsStore an environmentObject Fixes: maxgoedjen#536 (comment)

paulhdk added a commit to paulhdk/secretive that referenced this pull request Apr 6, 2024

Use SettingsStore for querying the comment style in SecretDetailView

c90d43a

Fixes: maxgoedjen#536 (comment)

paulhdk added a commit to paulhdk/secretive that referenced this pull request Apr 9, 2024

Make SettingsStore functions non-static, re-implement them as

ad6f2b0

subscripts and make settingsStore an environmentObject Fixes: maxgoedjen#536 (comment)

paulhdk added a commit to paulhdk/secretive that referenced this pull request Apr 9, 2024

Use SettingsStore for querying the comment style in SecretDetailView

834458f

Fixes: maxgoedjen#536 (comment)

ChristopherA mentioned this pull request Apr 27, 2024

Add detailed instructions for git and GitHub auth + signing #439

Open

paulhdk added a commit to paulhdk/secretive that referenced this pull request Jul 25, 2024

Make SettingsStore functions non-static, re-implement them as

35bd7ec

subscripts and make settingsStore an environmentObject Fixes: maxgoedjen#536 (comment)

paulhdk added a commit to paulhdk/secretive that referenced this pull request Jul 25, 2024

Use SettingsStore for querying the comment style in SecretDetailView

6709986

Fixes: maxgoedjen#536 (comment)

paulhdk added a commit to paulhdk/secretive that referenced this pull request Sep 16, 2024

Make SettingsStore functions non-static, re-implement them as

235fa1d

subscripts and make settingsStore an environmentObject Fixes: maxgoedjen#536 (comment)

paulhdk added a commit to paulhdk/secretive that referenced this pull request Sep 16, 2024

Use SettingsStore for querying the comment style in SecretDetailView

9d2165a

Fixes: maxgoedjen#536 (comment)

paulhdk added 3 commits September 15, 2024 19:32

Add SettingsStore

2a58323

Use keychain instead of UserDefaults in JustUpdatedChecker.swift

70cfdf8

Add ability disable the SSH comment via settings

4602296

paulhdk force-pushed the settings-rfc branch from 99124db to 4602296 Compare September 16, 2024 02:33

paulhdk added 6 commits September 29, 2024 16:11

Make SettingsStore functions non-static, re-implement them as

a5a3f1c

subscripts and make settingsStore an environmentObject Fixes: maxgoedjen#536 (comment)

Add rawValues for CommentStyle enum

5cbc4e5

Use SettingsStore for querying the comment style in SecretDetailView

e29dd20

Fixes: maxgoedjen#536 (comment)

Revert "Use keychain instead of UserDefaults in JustUpdatedChecker.sw…

52f3fea

…ift" This reverts commit ae8a21a.

Remove copyright info

748fc4e

Use "enum Constants" in SettingsStore

77bf72e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] Keychain-backed settings #536

[RFC] Keychain-backed settings #536

paulhdk commented Feb 29, 2024

maxgoedjen left a comment

maxgoedjen Mar 1, 2024

maxgoedjen Mar 1, 2024

maxgoedjen Mar 1, 2024

maxgoedjen Mar 1, 2024

paulhdk Apr 8, 2024 •

edited

Loading

paulhdk Apr 28, 2024

maxgoedjen Apr 28, 2024

maxgoedjen Mar 1, 2024

maxgoedjen Mar 1, 2024

maxgoedjen Mar 1, 2024

maxgoedjen Mar 1, 2024

maxgoedjen Mar 1, 2024

maxgoedjen commented Mar 1, 2024

paulhdk commented Mar 4, 2024

ChristopherA commented Apr 27, 2024

maxgoedjen commented Apr 27, 2024

ChristopherA commented Apr 27, 2024

ChristopherA commented Apr 27, 2024

maxgoedjen commented Apr 27, 2024

ChristopherA commented Apr 27, 2024

paulhdk commented Jul 25, 2024

[RFC] Keychain-backed settings #536

Are you sure you want to change the base?

[RFC] Keychain-backed settings #536

Conversation

paulhdk commented Feb 29, 2024

This pull request adds:

Here's how the Settings looks:

Concrete questions I have:

maxgoedjen left a 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

paulhdk Apr 8, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxgoedjen commented Mar 1, 2024

paulhdk commented Mar 4, 2024

ChristopherA commented Apr 27, 2024

maxgoedjen commented Apr 27, 2024

ChristopherA commented Apr 27, 2024

ChristopherA commented Apr 27, 2024

maxgoedjen commented Apr 27, 2024

ChristopherA commented Apr 27, 2024

paulhdk commented Jul 25, 2024

paulhdk Apr 8, 2024 •

edited

Loading