Password manager: Keeper Security integration via Commander Service Mode API#625
Password manager: Keeper Security integration via Commander Service Mode API#625hborase-ks wants to merge 40 commits intognachman:masterfrom
Conversation
… key inside in-memory storage
… + upstream entries) Made-with: Cursor
Made-with: Cursor
…flict Made-with: Cursor
… + upstream new files) Made-with: Cursor
|
Did you consider using the plugin architecture? See https://github.com/gnachman/iTerm2/tree/master/pwmplugin. It makes things a bit simpler because of the clean boundary between the main app and the password manager-specific code. |
|
Thanks for working on Keeper support! I'd like to get this integrated but the PR needs to be reworked to use the generic adapter protocol extensions I've added in commit f456e5f. What changed on masterI extended the password manager adapter protocol so that features like Keeper's can be declared entirely through the handshake response, without any
What this means for the PRThe following files from your PR should not be needed and should be removed:
What should remain:
Other issues from the original PR that still apply
How to testThere's a dev-only test adapter ( Happy to help if you have questions about the new protocol fields. |
|
Hi @gnachman, Thanks for the detailed guidance and for f456e5f—that clears up the architecture. I’ve reworked the PR accordingly: Keeper is handshake-only (pwmplugin/…/main.swift + prebuilt binary as needed), removed KeeperDataSource.swift, no Keeper-specific checks or hand-built settings/sync UI in host code, and the adapter uses header.settings. I also reverted local pbxproj signing/bundle ID changes, addressed the command-injection note for record UIDs, dropped the extra keychain entitlement if generic storage covers it, and reverted unrelated whitespace. Happy to adjust anything that still looks out of scope. |
gnachman
left a comment
There was a problem hiding this comment.
Thanks for adding Keeper Security support! The adapter itself (the pwmplugin/Sources/iterm2-keeper-adapter/ code) looks good and follows the established pattern. However, the PR includes a number of main-app changes that need to be pulled back. The only main-app changes needed for this PR are:
- Adding the Keeper menu item to the password manager window controller's menu (the xib change)
- Registering the adapter in
PasswordManagerDataSourceProvider
Everything else in the main app should be removed from this PR. Here's the specific list:
Remove from this PR
-
bridgedMasterPasswordSettingsFieldKey/bridgedPathToDatabaseSettingsFieldKeyinAdapterPasswordDataSource.swift— The adapter protocol already has settings fields infrastructure. These heuristic shims shouldn't be needed; the existing generic settings flow should handle Keeper's settings without special-casing in the host. -
get-usernamerecipe and all related plumbing — This includesmakeGetUsernameRecipe()inAdapterPasswordDataSource,getUsernameRecipeinCommandLinePasswordDataSource.Configuration,usernameForTerminal(context:completion:)on thePasswordManagerAccountprotocol,GetUsernameResponseinPasswordManagerProtocol.swift, and the implementations inKeychainPasswordDataSource. See question below about usernames. -
skipKeeperTouchIDGateonRecipeExecutionContext— This is declared but never read anywhere. Remove it. -
Empty password validation in
handleEditPasswordCompletion:row:newPassword:— There's no reason to prevent empty passwords. A user may want to store only a username. -
Success alert dialogs (
showPasswordUpdatedSuccessfully…,showDeleteRecordSuccessfully…,showAddRecordSuccessfully…) — No other password manager shows modal success alerts. The table reload is sufficient feedback. If you want success feedback, it should be less invasive (and that would be a separate change affecting all managers). -
enterUsernameAndCloseAfterSending:refactor — This is a regression. Previously, clicking "Enter Username" sent the username and left the window open so you could still send the password. The new code passescloseAfterSending:YESfromperformSecondaryAction:, which closes the window after sending the username. Please revertenterUsernameto its original implementation. -
Unrelated whitespace changes — Trailing newline removals in
TransferrableFile.handIntervalTreeGraphEncodingTests.swiftshould not be in this PR.
Question about usernames
Why doesn't the adapter populate userName correctly in the list-accounts response? The UI expects it and the username column would look wrong without it. If there's a performance issue with fetching login fields during listing, we'd need to discuss an alternative approach — but that would be a separate PR. For this PR, please make list-accounts return proper usernames if at all possible.
Adapter code
The adapter code in pwmplugin/ looks solid. One thing to double-check: in addRecord (KeeperCommanderClient.swift), the escaping of accountName and userName only handles \ and ". If Commander Service Mode does any shell-like parsing of the command string, additional characters may need escaping.
…sed username retrieval - Updated README to clarify `list-accounts` command behavior regarding `userName`. - Removed `get-username` functionality and associated code to reduce API calls and avoid rate limits. - Enhanced `listUserName` logic in `KeeperRecord` to derive usernames from available fields. - Adjusted tests to reflect changes in account listing and username handling. - Cleaned up unused methods and variables across various source files for better maintainability.
|
@gnachman Thanks for the review — here’s what we changed to match your notes. Main app — removed from this PR (reverted / dropped)
List userName (your question) Adapter Let us know if anything still looks out of scope. |
Summary
Adds Keeper Password Manager as a first-class password manager option in iTerm2, alongside macOS Keychain, 1Password, LastPass, KeePassXC, and Bitwarden.
Architecture
Changes
sources/KeeperDataSource.swift— newPasswordManagerDataSourceimplementation for KeeperModernTests/KeeperDataSourceTests.swift— unit tests for parsing, API flows, Keychain storage, and error handlingPrerequisites
Security notes
References