Skip to content
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

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from
9 changes: 9 additions & 0 deletions Sources/Secretive.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,8 @@
50BB046B2418AAAE00D6E079 /* EmptyStoreView.swift in Sources */ = {isa = PBXBuildFile; fileRef = 50BB046A2418AAAE00D6E079 /* EmptyStoreView.swift */; };
50C385A52407A76D00AF2719 /* SecretDetailView.swift in Sources */ = {isa = PBXBuildFile; fileRef = 50C385A42407A76D00AF2719 /* SecretDetailView.swift */; };
50E9CF422B51D596004AB36D /* Localizable.xcstrings in Resources */ = {isa = PBXBuildFile; fileRef = 500B93C22B478D8400E157DE /* Localizable.xcstrings */; };
DA140C502B8DF70500948F81 /* SettingsHelper.swift in Sources */ = {isa = PBXBuildFile; fileRef = DA140C4F2B8DF70500948F81 /* SettingsHelper.swift */; };
DA22A3402B712A57004D45DD /* SettingsView.swift in Sources */ = {isa = PBXBuildFile; fileRef = DA22A33F2B712A57004D45DD /* SettingsView.swift */; };
/* End PBXBuildFile section */

/* Begin PBXContainerItemProxy section */
Expand Down Expand Up @@ -152,6 +154,9 @@
50B8550C24138C4F009958AC /* DeleteSecretView.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = DeleteSecretView.swift; sourceTree = "<group>"; };
50BB046A2418AAAE00D6E079 /* EmptyStoreView.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = EmptyStoreView.swift; sourceTree = "<group>"; };
50C385A42407A76D00AF2719 /* SecretDetailView.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SecretDetailView.swift; sourceTree = "<group>"; };
DA140C4F2B8DF70500948F81 /* SettingsHelper.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SettingsHelper.swift; sourceTree = "<group>"; };
DA22A33C2B6EB835004D45DD /* SecretiveRelease.entitlements */ = {isa = PBXFileReference; lastKnownFileType = text.plist.entitlements; path = SecretiveRelease.entitlements; sourceTree = "<group>"; };
DA22A33F2B712A57004D45DD /* SettingsView.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SettingsView.swift; sourceTree = "<group>"; };
/* End PBXFileReference section */

/* Begin PBXFrameworksBuildPhase section */
Expand Down Expand Up @@ -192,6 +197,7 @@
isa = PBXGroup;
children = (
50033AC227813F1700253856 /* BundleIDs.swift */,
DA140C4F2B8DF70500948F81 /* SettingsHelper.swift */,
);
path = Helpers;
sourceTree = "<group>";
Expand Down Expand Up @@ -282,6 +288,7 @@
50153E1F250AFCB200525160 /* UpdateView.swift */,
5066A6C12516F303004B5A36 /* SetupView.swift */,
5066A6C72516FE6E004B5A36 /* CopyableView.swift */,
DA22A33F2B712A57004D45DD /* SettingsView.swift */,
);
path = Views;
sourceTree = "<group>";
Expand Down Expand Up @@ -497,6 +504,7 @@
5079BA0F250F29BF00EA86F4 /* StoreListView.swift in Sources */,
50617DD223FCEFA90099B055 /* PreviewStore.swift in Sources */,
5066A6F7251829B1004B5A36 /* ShellConfigurationController.swift in Sources */,
DA22A3402B712A57004D45DD /* SettingsView.swift in Sources */,
50033AC327813F1700253856 /* BundleIDs.swift in Sources */,
508A58B3241ED2180069DC07 /* AgentStatusChecker.swift in Sources */,
50C385A52407A76D00AF2719 /* SecretDetailView.swift in Sources */,
Expand All @@ -508,6 +516,7 @@
50BB046B2418AAAE00D6E079 /* EmptyStoreView.swift in Sources */,
50617D8323FCE48E0099B055 /* App.swift in Sources */,
506772C92425BB8500034DED /* NoStoresView.swift in Sources */,
DA140C502B8DF70500948F81 /* SettingsHelper.swift in Sources */,
50153E22250DECA300525160 /* SecretListItemView.swift in Sources */,
508A58B5241ED48F0069DC07 /* PreviewAgentStatusChecker.swift in Sources */,
508A58AA241E06B40069DC07 /* PreviewUpdater.swift in Sources */,
Expand Down
5 changes: 5 additions & 0 deletions Sources/Secretive/App.swift
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ struct Secretive: App {
}()
private let agentStatusChecker = AgentStatusChecker()
private let justUpdatedChecker = JustUpdatedChecker()
private let settingsStore = SettingsStore()

@AppStorage("defaultsHasRunSetup") var hasRunSetup = false
@State private var showingSetup = false
Expand All @@ -27,6 +28,7 @@ struct Secretive: App {
.environmentObject(storeList)
.environmentObject(Updater(checkOnLaunch: hasRunSetup))
.environmentObject(agentStatusChecker)
.environmentObject(settingsStore)
.onAppear {
if !hasRunSetup {
showingSetup = true
Expand Down Expand Up @@ -62,6 +64,9 @@ struct Secretive: App {
}
SidebarCommands()
}
Settings {
SettingsView()
}
}

}
Expand Down
58 changes: 58 additions & 0 deletions Sources/Secretive/Helpers/SettingsHelper.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@

import Foundation

class SettingsStore: ObservableObject {
enum Constants {
static let service = "com.maxgoedjen.Secretive"
}
}

extension SettingsStore {
subscript(key: String) -> String? {
set(value) {
guard let valueData = value?.data(using: String.Encoding.utf8)! else {
return
}

if let keyVal = self[key] {
if keyVal == value {
return
}

let updateQuery: [String: Any] = [kSecClass as String: kSecClassGenericPassword,
kSecAttrServer as String: Constants.service]
let attributes: [String: Any] = [kSecAttrAccount as String: key,
kSecValueData as String: valueData]
// FIXME: Make this non-blocking as described here: https://developer.apple.com/documentation/security/1393617-secitemupdate
let status = SecItemUpdate(updateQuery as CFDictionary, attributes as CFDictionary)
guard status == errSecSuccess else {
print("Couldn't update item in keychain. " + status.description)
return
}
} else {
let addquery: [String: Any] = [kSecClass as String: kSecClassGenericPassword,
kSecAttrAccount as String: key,
kSecAttrServer as String: Constants.service,
kSecValueData as String: valueData]
// FIXME: Make this non-blocking as described here: https://developer.apple.com/documentation/security/1401659-secitemadd
let status = SecItemAdd(addquery as CFDictionary, nil)
guard status == errSecSuccess else {
print("Couldn't add item to keychain. " + status.description)
return
}
}
}

get {
let getquery: [String: Any] = [kSecClass as String: kSecClassGenericPassword,
kSecAttrAccount as String: key,
kSecAttrServer as String: Constants.service,
kSecMatchLimit as String: kSecMatchLimitOne,
kSecReturnData as String: true]
var item: CFTypeRef?
let status = SecItemCopyMatching(getquery as CFDictionary, &item)

return status == errSecSuccess ? String(decoding: item as! Data, as: UTF8.self) : nil
}
}
}
15 changes: 15 additions & 0 deletions Sources/Secretive/Localizable.xcstrings
Original file line number Diff line number Diff line change
Expand Up @@ -2510,6 +2510,9 @@
}
}
}
},
"General" : {

},
"no_secure_storage_description" : {
"localizations" : {
Expand Down Expand Up @@ -2702,6 +2705,9 @@
}
}
}
},
"None" : {

},
"persist_authentication_accept_button" : {
"comment" : "When the user authorizes an action using a secret that requires unlock, they're shown a notification offering to leave the secret unlocked for a set period of time. This is the title for the notification.",
Expand Down Expand Up @@ -3415,6 +3421,9 @@
}
}
}
},
"Settings" : {

},
"setup_agent_activity_monitor_description" : {
"localizations" : {
Expand Down Expand Up @@ -4464,6 +4473,12 @@
}
}
}
},
"SSH Public Key Comment" : {

},
"SSH public keys can be extended with an arbitrary comment string without changing the meaning of the key." : {

},
"unnamed_secret" : {
"extractionState" : "manual",
Expand Down
10 changes: 8 additions & 2 deletions Sources/Secretive/Views/SecretDetailView.swift
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import SecretKit
struct SecretDetailView<SecretType: Secret>: View {

@State var secret: SecretType
@EnvironmentObject private var settingsStore: SettingsStore

private let keyWriter = OpenSSHKeyWriter()
private let publicKeyFileStoreController = PublicKeyFileStoreController(homeDirectory: NSHomeDirectory().replacingOccurrences(of: Bundle.main.hostBundleID, with: Bundle.main.agentBundleID))
Expand Down Expand Up @@ -42,9 +43,14 @@ struct SecretDetailView<SecretType: Secret>: View {
}

var keyString: String {
keyWriter.openSSHString(secret: secret, comment: "\(dashedKeyName)@\(dashedHostName)")
var style: CommentStyle = CommentStyle(rawValue: settingsStore["com.maxgoedjen.Secretive.commentStyle"] ?? CommentStyle.keyAndHost.rawValue)!
switch style {
Copy link
Owner

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

Copy link
Contributor Author

@paulhdk paulhdk Apr 8, 2024

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 🙂

Copy link
Contributor Author

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 🙂

Copy link
Owner

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

case .none:
return keyWriter.openSSHString(secret: secret, comment: "")
case .keyAndHost:
return keyWriter.openSSHString(secret: secret, comment: "\(dashedKeyName)@\(dashedHostName)")
}
}

}

#if DEBUG
Expand Down
52 changes: 52 additions & 0 deletions Sources/Secretive/Views/SettingsView.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@

import SwiftUI

enum CommentStyle: String, CaseIterable, Identifiable {
case keyAndHost = "keyAndHost"
case none = "none"

var id: Self { self }
}

struct GeneralSettingsView: View {
@AppStorage("com.maxgoedjen.Secretive.commentStyle") var selectedCommentStyle: CommentStyle = .keyAndHost
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as secretdetailview, need to make sure this makes it to secure store not just defaults.


var body: some View {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UI looks nice 👏

VStack(alignment: .leading) {
Section(footer: Text("SSH public keys can be extended with an arbitrary comment string without changing the meaning of the key.")
.font(.caption)
.fontWeight(.light)) {
Picker("SSH Public Key Comment", selection: $selectedCommentStyle) {
Text("Default").tag(CommentStyle.keyAndHost)
Text("None").tag(CommentStyle.none)
}
.pickerStyle(DefaultPickerStyle())
}
}
.padding(20)
.frame(width: 350, height: 100)
.navigationTitle("Settings")
}
}


struct SettingsView: View {
private enum Tabs: Hashable {
case general
}
var body: some View {
TabView {
GeneralSettingsView()
.tabItem {
Label("General", systemImage: "gear")
}
.tag(Tabs.general)
}
.padding(20)
.frame(width: 500, height: 200)
}
}

#Preview {
SettingsView()
}