Skip to content

Password manager: Keeper Security integration via Commander Service Mode API#625

Open
hborase-ks wants to merge 40 commits intognachman:masterfrom
hborase-ks:master
Open

Password manager: Keeper Security integration via Commander Service Mode API#625
hborase-ks wants to merge 40 commits intognachman:masterfrom
hborase-ks:master

Conversation

@hborase-ks
Copy link
Copy Markdown

Summary

Adds Keeper Password Manager as a first-class password manager option in iTerm2, alongside macOS Keychain, 1Password, LastPass, KeePassXC, and Bitwarden.

Architecture

  • Connects to a Keeper Commander Service Mode endpoint via REST API v2 (queue-enabled).
  • User configures API URL and API token in the Keeper Security Settings sheet.
  • API token is stored in macOS Keychain with optional biometric (Touch ID) protection.
  • Supports list, get, add, update, delete, and sync-down vault operations.

Changes

  • sources/KeeperDataSource.swift — new PasswordManagerDataSource implementation for Keeper
  • ModernTests/KeeperDataSourceTests.swift — unit tests for parsing, API flows, Keychain storage, and error handling
  • Provider/UI wiring so Keeper is selectable in Password Manager

Prerequisites

  • Keeper Commander Service Mode must be running and reachable from the machine where iTerm2 runs:
    • Locally (e.g. on the same Mac), or
    • Remotely via an integrated Ngrok or Cloudflare tunnel (or equivalent deployment described in Keeper’s Service Mode docs).
  • Command access in Service Mode must allow the operations iTerm2 needs (configure the allowed command list accordingly; restrict to the minimum required).
  • User must have a valid Keeper vault accessible through that Commander session.

Security notes

  • This integration only talks to the Commander Service Mode URL the user configures; iTerm2 does not embed Keeper credentials or run Commander.
  • The API token and service URL are stored in macOS Keychain (with Touch ID / device protection when available for the token).
  • Access control and command allowlists are enforced by Keeper Commander Service Mode on the configured endpoint — users should run with the smallest command set that still supports password manager actions.

References

@gnachman
Copy link
Copy Markdown
Owner

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.

@gnachman
Copy link
Copy Markdown
Owner

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 master

I extended the password manager adapter protocol so that features like Keeper's can be declared entirely through the handshake response, without any identifier == "Keeper Security" checks in the host code. The key additions to HandshakeResponse:

  • pathToDatabaseKind (.file or .url) — use .url so iTerm2 shows a text field instead of a file picker for the Commander API URL.
  • pathToDatabasePrompt / pathToDatabasePlaceholder — custom prompt and placeholder for the URL field.
  • masterPasswordLabel — set to "API key" so the prompt says "Enter API key for Keeper Security:" instead of "master password."
  • persistsCredentials — when true, iTerm2 stores the master password (API key) in the keychain after a successful login and auto-restores it on next launch. No need for your own keychain code in KeeperDataSource.swift.
  • settingsFields — declare custom configuration fields (key, label, secret, keychain-backed, etc.) and iTerm2 builds a settings sheet automatically. Values are passed to the adapter in header.settings on every request. This replaces the hand-built Keeper settings sheet in iTermPasswordManagerWindowController.m.
  • customCommands — declare commands like sync-down with a label and icon, and they appear in the gear menu automatically. This replaces the Keeper-specific sync button and runKeeperSyncDown bridge.

What this means for the PR

The following files from your PR should not be needed and should be removed:

  • sources/KeeperDataSource.swift — all keychain/URL storage, migration, and dialog code is handled generically now.
  • All Keeper-specific code in sources/AdapterPasswordDataSource.swift — no identifier == "Keeper Security" checks.
    The adapter just declares its capabilities in the handshake.
  • All Keeper-specific code in sources/iTermPasswordManagerWindowController.m — no settings sheet code, no sync button
    code, no connection notifications, no iTermKeeperSettingsAccessoryContext, etc.

What should remain:

  • pwmplugin/Sources/iterm2-keeper-adapter/ — the adapter CLI. Update the handshake response to declare the new fields. Read settings from header.settings instead of expecting them to be passed separately.
  • pwmplugin/Package.swift — the keeper adapter target.
  • sources/PasswordManagerDataSourceProvider.swift — add the Keeper case (following the pattern of KeePassXC/Bitwarden).
  • Interfaces/iTermPasswordManager.xib — add the "Keeper Security" menu item (no settings button or sync button needed; those are dynamic now).

Other issues from the original PR that still apply

  • project.pbxproj: Your PR changed DEVELOPMENT_TEAM from H7V7XYVQ7D to V75A2JBVMH and PRODUCT_BUNDLE_IDENTIFIER from com.googlecode.iterm2 to com.hiteshborase.iterm2.local across dozens of build configurations. These are your local signing settings and must be reverted. Only the file reference additions for the keeper adapter should remain.
  • iTerm2.entitlements: The keychain-access-groups addition should not be needed now that credential storage uses the generic SSKeychain path.
  • Command injection: In KeeperCommanderClient.swift, record UIDs are interpolated directly into command strings ("get \(recordUid) --format=json"). Validate that UIDs match the expected format before interpolation.
  • Unrelated whitespace changes: IntervalTreeGraphEncodingTests.swift and TransferrableFile.h — please revert.

How to test

There's a dev-only test adapter (iterm2-test-adapter) behind #if ITERM_DEBUG that exercises all the new protocol features. You can look at its source in pwmplugin/Sources/iterm2-test-adapter/main.swift as a reference for how to declare settings fields, custom commands, credential persistence, and URL-based database paths in the handshake.

Happy to help if you have questions about the new protocol fields.

@hborase-ks
Copy link
Copy Markdown
Author

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.

Copy link
Copy Markdown
Owner

@gnachman gnachman left a comment

Choose a reason for hiding this comment

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

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:

  1. Adding the Keeper menu item to the password manager window controller's menu (the xib change)
  2. 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 / bridgedPathToDatabaseSettingsFieldKey in AdapterPasswordDataSource.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-username recipe and all related plumbing — This includes makeGetUsernameRecipe() in AdapterPasswordDataSource, getUsernameRecipe in CommandLinePasswordDataSource.Configuration, usernameForTerminal(context:completion:) on the PasswordManagerAccount protocol, GetUsernameResponse in PasswordManagerProtocol.swift, and the implementations in KeychainPasswordDataSource. See question below about usernames.

  • skipKeeperTouchIDGate on RecipeExecutionContext — 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 passes closeAfterSending:YES from performSecondaryAction:, which closes the window after sending the username. Please revert enterUsername to its original implementation.

  • Unrelated whitespace changes — Trailing newline removals in TransferrableFile.h and IntervalTreeGraphEncodingTests.swift should 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.
@hborase-ks
Copy link
Copy Markdown
Author

@gnachman Thanks for the review — here’s what we changed to match your notes.

Main app — removed from this PR (reverted / dropped)

  • bridgedMasterPasswordSettingsFieldKey / bridgedPathToDatabaseSettingsFieldKey in AdapterPasswordDataSource.swift

  • All get-username plumbing: makeGetUsernameRecipe(), getUsernameRecipe in CommandLinePasswordDataSource.Configuration, usernameForTerminal… on PasswordManagerAccount, GetUsernameResponse in PasswordManagerProtocol, and the Keychain path

  • skipKeeperTouchIDGate on RecipeExecutionContext

  • Empty-password rejection in handleEditPasswordCompletion:row:newPassword:

  • Success modal alerts (showPasswordUpdated…, showDelete…, showAdd…)

  • enterUsernameAndCloseAfterSending: — restored Enter Username to the previous behavior: synchronous userName, window stays open so the password can still be sent

  • Whitespace — put back trailing newlines in TransferrableFile.h and ModernTests/IntervalTreeGraphEncodingTests.swift
    Main app — kept as the only wiring you asked for
    Keeper item in the password manager menu (xib) and registration in PasswordManagerDataSourceProvider (no extra main-app special cases beyond that).

List userName (your question)
The Keeper adapter now sets userName in list-accounts from the ls / list payload when available (login / username in JSON, else parsing the list description), without a per-record get to avoid Commander 429s.

Adapter
pwmplugin sources, tests, README, and rebuilt iterm2-keeper-adapter binary updated accordingly. We noted your point on addRecord escaping (\ / " only) for a possible follow-up if Commander does broader shell-like parsing.

Let us know if anything still looks out of scope.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants