Repository navigation
fix(connections): keep secrets in the keychain by default and write config files 0600 - #980
Conversation
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| @@ -1548,8 +1612,12 @@ pub async fn update_connection<R: Runtime>( | |||
| } | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
WARNING: Orphaned keychain entries if URI write fails after password fallback
In update_connection, password keychain operations run before persist_connection_uri_change (line 1614). If URI write fails (keychain unavailable), the entire save fails but any password keychain entries that succeeded are left orphaned — the connection file is never saved. Consider running URI persistence first, or rolling back password keychain writes on URI failure.
There was a problem hiding this comment.
good catch, fixed in 946c76a. save and update now track which keychain entries they wrote (plus the old value on update) and put them back if the config/URI write fails, so a failed save doesn't leave anything behind
| use std::io::Write; | ||
| use std::os::unix::fs::{OpenOptionsExt, PermissionsExt}; | ||
|
|
||
| if path.exists() { |
There was a problem hiding this comment.
SUGGESTION: Redundant permission setting in write_private_file
On Unix, this function sets permissions three times:
- Line 88-89:
set_permissionson existing file - Line 95:
mode(0o600)inOpenOptions(creates file with 0600) - Line 100:
set_permissionsagain after write
The mode(0o600) at open already ensures the new file has correct permissions. The first set_permissions is only needed for existing files, and the final one is redundant. Consider simplifying to: set permissions on existing file if needed, then open with mode(0o600) and write.
There was a problem hiding this comment.
yep the last set_permissions was redundant, dropped it. kept the one before open though since mode() only kicks in when the file is created, so an existing 0644 file still needs tightening before the secrets get written
| @@ -366,6 +368,11 @@ export const NewConnectionModal = ({ | |||
| ssh_enabled: false, | |||
| ssh_port: 22, | |||
| k8s_enabled: false, | |||
| // New connections store secrets in the keychain. An edit starts from the | |||
| // saved flag so a slow credential load cannot flip an existing connection. | |||
| save_in_keychain: initialConnection | |||
There was a problem hiding this comment.
SUGGESTION: Explicit default for existing connection's save_in_keychain
initialConnection.params.save_in_keychain can be undefined (not set in saved JSON). The checkbox uses !!formData.save_in_keychain which coerces undefined to false, but this is implicit. For clarity and to match the comment "An edit starts from the saved flag", consider: save_in_keychain: initialConnection ? initialConnection.params.save_in_keychain ?? false : true,
There was a problem hiding this comment.
done, switched to ?? false
Code Review SummaryThis review did not finish. The model reached its output limit before it Previous Review Summaries (2 snapshots, latest commit 946c76a)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 946c76a)Status: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (1 file)
Fix these issues in Kilo Cloud Previous review (commit 5c99af5)Status: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (17 files)
|
5c99af5 to
946c76a
Compare
|
on the SSH/K8s fallback point, I left those alone on purpose. this PR only swaps their fs::write for write_private_file, the keychain handling there is the same as main (SSH already defaulted to keychain before this) and K8s connections don't keep anything in the keychain. a plaintext fallback there feels like its own change, happy to do a follow up if you want it |
| ssh_connections.push(ssh_to_save.clone()); | ||
| let json = serde_json::to_string_pretty(&ssh_connections).map_err(|e| e.to_string())?; | ||
| fs::write(path, json).map_err(|e| e.to_string())?; | ||
| persistence::write_private_file(&path, json)?; |
There was a problem hiding this comment.
WARNING: Orphaned keychain entries if SSH config write fails after keychain writes
save_ssh_connection stores SSH password/passphrase in the keychain (lines ~2155-2177) before writing the SSH config file with write_private_file (this line). If the file write fails (disk full, permissions, etc.), the keychain entries are left orphaned — the SSH connection is never saved but secrets remain in the keychain. Apply the same KeychainSecretWrites rollback pattern used in save_connection/update_connection.
|
|
||
| let json = serde_json::to_string_pretty(&ssh_connections).map_err(|e| e.to_string())?; | ||
| fs::write(path, json).map_err(|e| e.to_string())?; | ||
| persistence::write_private_file(&path, json)?; |
There was a problem hiding this comment.
WARNING: Orphaned keychain entries if SSH config write fails after keychain writes
update_ssh_connection stores SSH password/passphrase in the keychain (lines ~2210-2220) before writing the SSH config file with write_private_file (this line). If the file write fails, the keychain entries are left orphaned. Apply the same KeychainSecretWrites rollback pattern used in save_connection/update_connection.
| save_connections_and_invalidate(&app, &conn_path, ¤t_file)?; | ||
| let ssh_json = serde_json::to_string_pretty(¤t_ssh).map_err(|e| e.to_string())?; | ||
| fs::write(ssh_path, ssh_json).map_err(|e| e.to_string())?; | ||
| persistence::write_private_file(&ssh_path, ssh_json)?; |
There was a problem hiding this comment.
WARNING: Orphaned keychain entries if SSH config write fails after keychain writes
apply_export_payload writes multiple keychain entries (DB passwords, connection URIs, SSH passwords, SSH passphrases — lines ~6903-6960) before saving the SSH config file with write_private_file (this line). If the file write fails, all those keychain entries are left orphaned. This affects bulk import where many secrets could be stranded. Consider tracking keychain writes per connection/SSH record and rolling back on failure, or persisting files first then moving secrets to keychain.
|
|
||
| let json = serde_json::to_string_pretty(&ssh_connections).map_err(|e| e.to_string())?; | ||
| fs::write(path, json).map_err(|e| e.to_string())?; | ||
| persistence::write_private_file(&path, json)?; |
There was a problem hiding this comment.
SUGGESTION: delete_ssh_connection deletes keychain entries before file write
Keychain entries are deleted (lines ~2278-2282) before the SSH config file is rewritten with write_private_file (this line). If the file write fails, the keychain secrets are already gone but the SSH connection record remains in the file — the user loses the secrets but keeps the (now unusable) connection entry. Consider deleting keychain entries only after the file write succeeds, or use a rollback pattern.
debba
left a comment
debba
left a comment
There was a problem hiding this comment.
Thanks @cestercian, really solid work on this one. I like that KeychainSecretWrites captures the previous keychain value before each write, so a failed config write puts back exactly what was there instead of blindly deleting. The two Unix tests for write_private_file (fresh file and tightening an existing 0644) are exactly what I wanted to see. Your reasoning on the SSH/K8s fallback makes sense to me too: keeping that out of this PR is the right call, and we can look at it separately if it turns out to matter.
I ran the modal and SSH manager tests (51 passing), the persistence tests (7 passing), the full Rust lib suite (the only failures are ones I also get on main on this machine), lint and typecheck. I'm approving. There are two small follow-ups:
src/components/ssh/SshConnectionsManager.tsx:570: the description mentions SSH manager test cases, buttests/components/ssh/SshConnectionsManager.test.tsxisn't touched, so the new plain-text warning there has no coverage. One test that unticks the box and checks the message (and thearia-describedby) would match what the PR says.src-tauri/src/commands.rs:1369and1390(same pattern at1587/1609inupdate_connection): if the DB password lands in the keychain but the SSH password or passphrase write then fails, we fall back to the file and clearsave_in_keychain. The DB password we just stored stays in the keychain as a stale copy, though. It gets cleaned on delete or on the next non-keychain save, so nothing breaks, but callingkeychain_writes.roll_backin the fallback branch would keep the keychain and the file consistent.
I'll still do my manual checks in the app before merging.
|
Pushed the follow-ups. There's a new SshConnectionsManager test that unticks the keychain box and checks the plain-text warning plus its |
debba
left a comment
debba
left a comment
There was a problem hiding this comment.
Thanks @cestercian, both follow-ups look good. The new SshConnectionsManager test covers exactly what was missing: it checks the warning isn't there while the box is ticked, then that it shows up with the right id and is wired through aria-describedby once you untick it.
On the fallback, you're right, and discard is better than what I suggested. Once the config file lands with the secrets inline and save_in_keychain set to false, nothing reads those keychain entries anymore, so restoring the previous values on update would only leave orphans behind. Deleting what this save wrote, and keeping roll_back for the case where the config write itself fails, keeps the two paths easy to follow.
On 34a4bca I ran the modal and SSH manager tests (52 passing), the persistence tests, lint, typecheck and clippy, all clean. The Rust lib suite only fails on environment-related tests on my machine that have nothing to do with this PR.
Approving. I'll merge as soon as CI is green.
Preview buildThe preview build of commit |
…ease) Resolves 21 conflict files across 38 upstream commits (v0.28.0 release): K8s tunnel health-check (TabularisDB#942): - commands.rs: upstream's reuse_or_discard_dead replaces our simple tunnel check; k8s connection files now use persistence::write_private_file (TabularisDB#980) Plugin RPC bounded shutdown (TabularisDB#961) + database routing (TabularisDB#960): - plugins/driver.rs: upstream's dropped_plugin_response_error, route_selected_plugin_database, accepting_calls, manager_task integrated with our with_primary_database (now preserves Multiple arrays for schema-less plugins so route_selected_plugin_database can route them) - Our is_method_not_found (PluginCallError-typed) kept over upstream's string-typed version; upstream's new helpers added alongside Window placement (TabularisDB#953) + e2e mock: - lib.rs: both our e2e_dialog_mock and upstream's window_placement module View definition context menu (TabularisDB#1010): - ExplorerSidebar.tsx: upstream's createViewDefinitionContextItems helper adopted; extended with our database param for multi-db support Editor MRU tab switching (TabularisDB#1011): - EditorContext.ts/EditorProvider.tsx: upstream's setActiveTabId preview option + getRecentTabIds merged with our database param on getSchema and findExistingTableTab NewConnectionModal keychain default (TabularisDB#980): - NewConnectionModal.tsx: our isSchemaBasedMultiDbCapable + hasOptedIntoDatabaseSelection + upstream's withoutBlankSslMode import - NewConnectionModal.test.tsx: both our multi-database opt-in tests and upstream's keychain default tests preserved Theme test tempdir canonicalization: - 14 theme_packages test files: our temp_dir() kept (functionally identical to upstream's super::tempdir()); both solve the macOS /var -> /private/var symlink issue Struct field additions (TabularisDB#840/TabularisDB#939 numeric precision, TabularisDB#961 shutdown): - dump_commands.rs: numeric_precision/numeric_scale added to TableColumn test initializer - plugins/driver.rs test helpers: route_selected_database, accepting_calls, manager_task added to PluginProcess test initializers
Fixes #971
New database connections now default to storing the password in the OS keychain, same as the SSH manager already does. Editing an existing connection keeps whatever
save_in_keychainvalue it had.Config files that can hold secrets (
connections.json, the SSH and K8s connection files) go through a smallwrite_private_filehelper: mode0600on Unix, and an existing file with looser permissions gets tightened on the next save. Other platforms write as before.If the keychain checkbox is off, the new-connection form and the SSH manager show: "Passwords will be stored in plain text in the connection file." Same string is in all 11 locales. If a database save asked for the keychain and the OS refused the write, the connection still saves, the password stays in the config file,
save_in_keychainis stored as false, and that sentence shows as a warning. A connection URI is still refused unless the keychain flag is on.Left as normal writes: keybindings, preferences, query history, notebooks, logs, theme/plugin installer files, and similar non-secret paths. Nearby open PRs #934 and #779 touch keychain error reporting / SSH secret handling; this change does not copy from them and keeps the diff focused on the default + 0600 writer.
Tests:
cargo +stable test --locked --lib— 1581 passedpnpm lint/pnpm typecheck— passpnpm exec vitest runon the new modal + SSH manager tests — 51 passed