-
-
Notifications
You must be signed in to change notification settings - Fork 160
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[RFC] Keychain-backed settings #536
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ;)
@@ -4,6 +4,7 @@ import SecretKit | |||
struct SecretDetailView<SecretType: Secret>: View { | |||
|
|||
@State var secret: SecretType | |||
@AppStorage("com.maxgoedjen.Secretive.commentStyle") var style: CommentStyle = .keyAndHost |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just going to be pulling directly from UserDefaults, we want this plugged into the SettingsStore
instead.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep these in UserDefaults as-is for now, these don't need to be in secure storage.
switch style { | ||
case CommentStyle.none: | ||
keyWriter.openSSHString(secret: secret, comment: "") | ||
default: | ||
keyWriter.openSSHString(secret: secret, comment: "\(dashedKeyName)@\(dashedHostName)") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)") | |
} |
@@ -42,9 +43,13 @@ struct SecretDetailView<SecretType: Secret>: View { | |||
} | |||
|
|||
var keyString: String { | |||
keyWriter.openSSHString(secret: secret, comment: "\(dashedKeyName)@\(dashedHostName)") | |||
switch style { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maxgoedjen just giving this a polite bump in case it got lost 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
// | ||
// SettingsView.swift | ||
// Secretive | ||
// | ||
// Created by Paul Heidekrüger on 05.02.24. | ||
// Copyright © 2024 Max Goedjen. All rights reserved. | ||
// | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// | |
// SettingsView.swift | |
// Secretive | |
// | |
// Created by Paul Heidekrüger on 05.02.24. | |
// Copyright © 2024 Max Goedjen. All rights reserved. | |
// |
// | ||
// SettingsHelper.swift | ||
// Secretive | ||
// | ||
// Created by Paul Heidekrüger on 27.02.24. | ||
// Copyright © 2024 Max Goedjen. All rights reserved. | ||
// | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// | |
// SettingsHelper.swift | |
// Secretive | |
// | |
// Created by Paul Heidekrüger on 27.02.24. | |
// Copyright © 2024 Max Goedjen. All rights reserved. | |
// |
} | ||
|
||
extension SettingsStore { | ||
static func set(key: String, value: String) -> Bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably makes sense to have the core get/set methods her expose Data
directly (and maybe have a couple thin wrapper methods for string).
import Foundation | ||
|
||
class SettingsStore { | ||
static let service = "com.maxgoedjen.Secretive" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: move this into an enum Constants
like this: https://github.com/maxgoedjen/secretive/blob/main/Sources/Packages/Sources/SecureEnclaveSecretKit/SecureEnclaveStore.swift#L304-L312
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Answering your questions from PR:
|
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)
subscripts and make settingsStore an environmentObject Fixes: maxgoedjen#536 (comment)
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. |
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. |
I found the errant doc: https://github.com/maxgoedjen/secretive/pull/439/files
|
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. |
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. |
@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)
subscripts and make settingsStore an environmentObject Fixes: maxgoedjen#536 (comment)
99124db
to
4602296
Compare
subscripts and make settingsStore an environmentObject Fixes: maxgoedjen#536 (comment)
…ift" This reverts commit ae8a21a.
Hi @maxgoedjen !
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:
get
andset
functions.JustUpdatedChecker.swift
.File -> Settings...
orCMD + ,
.Here's how the Settings looks:
Concrete questions I have:
JustUpdatedChecker.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 🙂