diff --git a/CHANGELOG.md b/CHANGELOG.md index f9d112e..7d789d6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,26 @@ adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ### Added +- **Keys can live in 1Password.** A profile with `"key_store": "1password"` and + an `op_vault` keeps its private keys as SSH Key items in that vault, created + with the `op` CLI; only the public halves are written to disk. Its hosts render + `IdentityFile .pub` and `IdentityAgent` pointing at the 1Password agent. + `rotate` stages the replacement in the vault, verifies it through the agent, + and keeps one `.old` predecessor; `rollback` restores it. Items are + archived, never deleted. Rotating a key after switching its profile to + `1password` moves it into the vault. `validate` checks the item matches the + `.pub`, and `doctor` checks for `op`, the agent socket, and that the agent + serves each vault (`agent.toml`). Two 1Password profiles may not keep the same + key name in one vault, and 1Password keys must be `ed25519`. +- **Batch rotation.** `rotate` accepts several keys, profiles or host aliases, + `--category `, or `--all`, with `--dry-run` to list the batch. One + confirmation, backup and snapshot; each key rotates on its own, so one failure + leaves only that key as it was. +- **`key regen-pub`** rebuilds `.pub` files from the private key, or from the + vault item for a 1Password profile, for the same selections. It reports each + key as unchanged, created or rewritten, and never touches a private key. +- **`op://` secret references.** A provider token whose value starts `op://` is + read with `op read`, beside the existing `cmd:` form. - **Profiles can be grouped into categories.** A profile with `"category": "clients"` keeps its keys under `profiles/clients//` and renders its `Host` blocks into `profiles/clients/config`, which the root config Includes @@ -60,6 +80,10 @@ adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ### Changed +- **GitHub verify checks the account.** When a deploy's API token and an ssh + login are both available, GitHub must greet the key as the account the token + belongs to. A token for the wrong account (a personal token left in a machine + user's variable) used to add the key there and pass verification. - **`reconcile` refuses to mint a key that only changed directory.** A declared key found in the other layout (flat when the manifest says categorized, or the reverse) used to be planned as a fresh key, leaving two keys for one identity. diff --git a/README.md b/README.md index ccd2132..cd08385 100644 --- a/README.md +++ b/README.md @@ -87,9 +87,12 @@ else is in the [documentation index](#documentation). ### Recipes - [Two GitHub accounts on one machine](docs/recipes/two-github-accounts.md) — one hostname, two identities, neither able to act as the other. +- [Keep keys in 1Password](docs/recipes/keep-keys-in-1password.md) — private keys generated and rotated inside a vault; only the `.pub` on disk. +- [Use a GitHub machine user](docs/recipes/github-machine-user.md) — one bot key with read access to every org repo, rotated in one command. - [Group profiles into categories](docs/recipes/group-profiles-into-categories.md) — `internal`, `clients`, `others`: one directory and one `config` file each. - [Onboard an existing `~/.ssh`](docs/recipes/onboard-an-existing-ssh-dir.md) — bring a hand-built setup under management without regenerating anything. - [Rotate a key with no downtime](docs/recipes/rotate-a-key.md) — stage, verify, commit; and how to step back. +- [Rotate or repair many keys at once](docs/recipes/rotate-many-keys.md) — a category or everything in one batch, and rebuilding `.pub` files. - [Work against a sandbox](docs/recipes/dev-mode.md) — exercise every verb without touching your real `~/.ssh`. - [Back up, and restore onto a new machine](docs/recipes/back-up-and-restore.md) — the only path that recovers the same keys. - [Recover from a locked-out server](docs/recipes/recover-from-lockout.md) — break-glass with no working SSH. diff --git a/docs/architecture.md b/docs/architecture.md index bba55c9..7173518 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -244,6 +244,21 @@ layering test fails any `filepath.Join` that builds `profiles/` by han That is the defect categories expose: a hand-built path is right for a flat profile and silently one directory too high for a categorized one. +### Why can keys live in 1Password, and why per profile? + +A key file on disk is only as safe as the disk, and every copy of `~/.ssh` (a +backup, a bundle, a laptop) carries it. With `key_store: 1password` the private +key is generated inside 1Password and never read back: sshmgr writes only the +public half, and ssh reaches the key through the 1Password agent, selected by +that `.pub` with `IdentitiesOnly`. Rotation, the part people skip, stays one +command, and the vault keeps every predecessor in its Archive. + +The store is a profile setting, not a host one, because a key is shared by its +profile's hosts: a per-host field could have two hosts disagree about where one +key lives. Nothing else about a key changes with its store: its name, its +inventory record and its `IdentityFile` identity are the same, so moving a +profile between stores is a rotation, not a migration of state. + ### Why is there no single service object? v1 had a `SshManagerService` facade: 1356 lines, 54 diff --git a/docs/configuration.md b/docs/configuration.md index 3defffb..7abc9ed 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -96,6 +96,8 @@ guarantees each alias offers only its own key and trusts only its own host keys. "profiles": { "work": { "category": "others", // optional: profiles/others/work/ + profiles/others/config + "key_store": "file", // or "1password" (then op_vault is required) + "op_vault": null, // the 1Password vault holding this profile's keys "key_scope": "per_service", // or "shared" "key_name": null, // set only when key_scope == "shared" "hosts": [ @@ -131,6 +133,7 @@ guarantees each alias offers only its own key and trusts only its own host keys. | `expiry_check.debounce_hours` | `24` | min gap between inline banners | | `expiry_check.desktop_notify` | `true` | allow the scheduled desktop notification | | `global_options` | `{}` | ssh options emitted under `Host *` (e.g. `AddKeysToAgent`, macOS `UseKeychain`) | +| `onepassword_agent` | platform default | the 1Password SSH agent socket for `key_store: 1password` profiles (see [Key stores](#key-stores)) | ### `provider` / `token_env` @@ -176,6 +179,55 @@ Set it with `sshmgr profile add --category ` or `sshmgr profile edit --category `. The edit moves the key directory and updates the inventory. See [Group profiles into categories](recipes/group-profiles-into-categories.md). +## Key stores + +A profile's private keys live in one of two places, set by `key_store`: + +| `key_store` | Private key | On disk | Rendered host | +|---|---|---|---| +| `file` (or omitted) | `profiles/…/` | private key + `.pub` | `IdentityFile ` | +| `1password` | an SSH Key item titled `` in `op_vault` | `.pub` only | `IdentityFile .pub` + `IdentityAgent ""` | + +With `1password`, `reconcile` creates the key inside 1Password with the +[`op` CLI](https://developer.1password.com/docs/cli/) and writes only its public +half. ssh then asks the 1Password agent for the key matching that `.pub`, so the +private key never touches disk. `rotate` generates replacements in the vault too. +Needs `op` (2.20 or later) signed in, and the agent turned on in the 1Password +app (Settings > Developer > Use the SSH agent); `doctor` checks both. + +**The agent must serve the vault.** With no `agent.toml`, the 1Password SSH +agent offers only keys in your Personal, Private or Employee vault. Once +`~/.config/1Password/ssh/agent.toml` exists, it offers only what the file lists +(even an empty file overrides the default). For any other vault, or whenever the +file exists, list the vault as a whole: + +```toml +[[ssh-keys]] +vault = "Simtabi" +``` + +A whole-vault entry, not one per item: a rotation's `.staged` item has to be +served to be verified. `doctor` checks this for every vault a profile names. + +The agent socket defaults to the platform's path (macOS +`~/Library/Group Containers/2BUA8C4S2C.com.1password/t/agent.sock`, Linux +`~/.1password/agent.sock`); set `defaults.onepassword_agent` to override it. On +Windows no `IdentityAgent` is rendered, because Microsoft OpenSSH uses one fixed +pipe for every host. + +Two rules are enforced when the manifest loads: + +- **Key names are unique within a vault.** The item is titled with the key name, + so two 1Password profiles sharing a vault and a key name would share one key. +- **Keys are `ed25519`.** 1Password generates nothing else, so another + `key_type` on a 1Password profile is rejected rather than silently ignored. + +The store is per profile, not per host, so the hosts sharing a key cannot +disagree about where it lives. To move a profile into 1Password, set +`key_store` and run `sshmgr rotate` on each key: it generates the new key in the +vault and archives the old key file to `old/`. See +[Keep keys in 1Password](recipes/keep-keys-in-1password.md). + ## Key scope - `per_service` *(default)* - one key per host: smallest blast radius. @@ -222,10 +274,14 @@ runs at use-time and its trimmed stdout is the token (so it integrates with any secret manager and the secret never touches disk): ```sh -GH_TOKEN=cmd:op read op://Private/GitHub/token # 1Password CLI +GH_TOKEN=op://Private/GitHub/token # 1Password secret reference +GH_TOKEN=cmd:op read op://Private/GitHub/token # the same, spelled out GH_TOKEN=cmd:keyring get ssh-manager gh_token # OS keyring ``` +A value starting `op://` is read with `op read`, the whole reference as one +argument, so item and field names may contain spaces. + The command runs once per process (memoized) via the argv-only subprocess chokepoint - no shell, no injection. A failed/empty/timed-out command yields no token (the provider falls back to the manual paste path). Note: a literal token diff --git a/docs/features.md b/docs/features.md index dadd328..9b8079f 100644 --- a/docs/features.md +++ b/docs/features.md @@ -49,6 +49,7 @@ project-local mode. See [installation.md](installation.md) for the full layout. | Command | What it does | Tests | |---|---|---| | `key list [selector]` | Every key with its fingerprint, expiry, the hosts using it, and its state — including keys nothing references. | U | +| `key regen-pub [selectors...] [--category] [--all] [--dry-run]` | Rebuild `.pub` files from the private key or the 1Password item; reports unchanged / created / rewritten / skipped. Never changes a private key. | U | | `key add [--type] [--rotate-after-days] [--host] [--passphrase]` | Declare a key on a profile and mint it. Without `--host` the key is minted **unwired**: it exists and nothing uses it, which `doctor` then reports. | U | | `key delete [--purge] [--revoke] [--yes] [--no-key-backup]` | Remove the declaration and the inventory record. **Refuses while any host still resolves to the key** — a `Host` block pointing at a missing `IdentityFile` fails as a bare "Permission denied (publickey)" with nothing naming the cause. `--purge` also deletes the files. | U | | `clean [--dry-run] [--adopt]` | Prune stale `known_hosts` pins, stale inventory records and write residue. Only removes pins tagged `sshmgr`; `--adopt` opts an untagged pin that matches a manifest host into management, since an untagged pin is presumed to be yours. | U | @@ -72,6 +73,7 @@ project-local mode. See [installation.md](installation.md) for the full layout. |---|---|---| | `deploy [target]` | Install the public key on its target via the host's provider (GitHub/GitLab CLI, cloud-VPS REST, `generic-ssh` `ssh-copy-id`, or web-panel/manual) and record it. **Exits ≠0 if a target is unreachable.** | U·E·F | | `rotate [--yes] [--allow-unverified] [--passphrase]` | Zero-downtime staged rotation: stage → deploy → verify → archive (≤1 predecessor). Aborts cleanly (pulling the staged key back) if a target can't be verified or is unreachable. | U·E·F | +| `rotate [selectors...] [--category] [--all] [--dry-run]` | Rotate a batch: several keys, profiles or aliases, a category, or every key a host uses. One confirmation, backup and snapshot; each key rotates on its own and a failure leaves only that key untouched. | U | | `rollback [--yes]` | Restore the single `/old/` predecessor (re-deploy is best-effort; skips unreachable hosts). | U·E·F | | `load ` | Add the profile's keys to the agent (Keychain on macOS). | U·F | @@ -95,7 +97,7 @@ bounded SSH-level reachability probe first, so a down or VPN-gated host (includi | Command | What it does | Tests | |---|---|---| -| `profile add [--shared] [--key-name] [--category]` | Add a profile, optionally filed under `profiles//`. | U·F | +| `profile add [--shared] [--key-name] [--category]` | Add a profile, optionally filed under `profiles//`. A profile keeps its keys on disk, or in 1Password with `key_store: 1password` (see [Key stores](configuration.md#key-stores)). | U·F | | `profile edit [--key-scope] [--key-name] [--category]` | Change a profile in place. `--category` moves its key directory under `profiles//` (or back, with `""`) and re-points its inventory records. | U | | `profile delete [--yes] [--revoke] [--purge] [--no-key-backup]` | Remove a profile and its hosts; `--revoke` pulls its keys off their targets. | U·F | | `host add [-H] [-u] [-p] [--provider] [--token-env] [--key-name] [--tag]` | Add a host to a profile (alias is **positional**). | U·F | diff --git a/docs/recipes/github-machine-user.md b/docs/recipes/github-machine-user.md new file mode 100644 index 0000000..c1be56d --- /dev/null +++ b/docs/recipes/github-machine-user.md @@ -0,0 +1,61 @@ +# Use a GitHub machine user + +Give an organization one SSH key that can read every repository, and rotate it +in one command. + +A GitHub organization cannot hold an SSH key, and a deploy key belongs to +exactly one repository: GitHub refuses the same public key on a second one. A +machine user is an ordinary GitHub account used only by automation. Put it in a +team with **Read** on the repositories it needs, and its single key reaches all +of them. + +## Set it up + +1. Create the account (for example `simtabi-bot`) with an inbox you control, + and turn on two-factor authentication. +2. In the organization, create a team (for example `bots-read`) with **Read** + access to the repositories, and add the account to it. +3. Create a token for the machine user that can manage its SSH keys: a + fine-grained token with the account permission *Git SSH keys: read and + write*, or a classic token with `admin:public_key`. +4. Store the token where sshmgr can read it, in `~/.config/ssh-manager/.env`: + + ```sh + GH_TOKEN_SIMTABI=op://Simtabi/simtabi-bot/token + ``` + +5. Wire a host to it in the manifest, with `provider` and `token_env`: + + ```jsonc + {"alias": "github-simtabi", "hostname": "github.com", "user": "git", + "provider": "github", "token_env": "GH_TOKEN_SIMTABI", + "key_name": "simtabibot_github-ed25519"} + ``` + +6. Add the key to the account and check it: + + ```sh + sshmgr reconcile + sshmgr deploy simtabibot_github-ed25519 + ssh -T git@github-simtabi # Hi simtabi-bot! ... + ``` + +Use the alias in remotes: `git@github-simtabi:simtabi/ssh-manager.git`. + +## Rotate it + +```sh +sshmgr rotate simtabibot_github-ed25519 +``` + +The new key is added to the account, verified, and the old one removed through +the API. Verify also checks that GitHub greets the new key as the account the +token belongs to, so a token for the wrong account fails the rotation instead of +committing a key the alias cannot use. + +To keep the private key out of `~/.ssh` as well, give the profile +`key_store: 1password`; see [Keep keys in 1Password](keep-keys-in-1password.md). + +--- + +[← Docs index](../../README.md#documentation) diff --git a/docs/recipes/keep-keys-in-1password.md b/docs/recipes/keep-keys-in-1password.md new file mode 100644 index 0000000..b3ffead --- /dev/null +++ b/docs/recipes/keep-keys-in-1password.md @@ -0,0 +1,72 @@ +# Keep keys in 1Password + +Generate and rotate a profile's private keys inside a 1Password vault, so only +the public halves are ever on disk. + +You need the [1Password CLI](https://developer.1password.com/docs/cli/get-started/) +(`op` 2.20 or later), signed in, and the SSH agent turned on in the 1Password +app (Settings > Developer > Use the SSH agent). + +If the vault is not your Personal, Private or Employee vault, or you already +have a `~/.config/1Password/ssh/agent.toml`, tell the agent to serve it: + +```toml +# ~/.config/1Password/ssh/agent.toml +[[ssh-keys]] +vault = "Simtabi" +``` + +In the manifest, give the profile a store and a vault: + +```jsonc +"simtabibot": { + "category": "orgs", + "key_store": "1password", + "op_vault": "Simtabi", + "hosts": [ + {"alias": "github-simtabi", "hostname": "github.com", "user": "git", + "provider": "github", "token_env": "GH_TOKEN_SIMTABI", + "key_name": "simtabibot_github-ed25519"} + ] +} +``` + +```sh +sshmgr doctor # op found, agent socket present, vault served +sshmgr reconcile # creates the item, writes simtabibot_github-ed25519.pub +sshmgr validate # the item exists and matches the .pub +``` + +The rendered host points ssh at the public key and the 1Password agent: + +``` +Host github-simtabi + HostName github.com + User git + IdentityFile ~/.ssh/profiles/orgs/simtabibot/simtabibot_github-ed25519.pub + IdentityAgent "~/Library/Group Containers/2BUA8C4S2C.com.1password/t/agent.sock" + IdentitiesOnly yes +``` + +Rotate the same way as any key: + +```sh +sshmgr rotate simtabibot_github-ed25519 +``` + +The new key is created in the vault as `.staged`, deployed and verified, +then takes the live title, with the old item kept as `.old`. Items are +archived, never deleted. + +## Moving an existing profile into 1Password + +Set `key_store` and `op_vault` on the profile, then rotate each of its keys. The +rotation generates the replacement in the vault and archives the old key file +pair to `old/`. Until you rotate, `validate` fails the key, because ssh would ask +the 1Password agent for a key it does not hold. + +Full reference: [Key stores](../configuration.md#key-stores). + +--- + +[← Docs index](../../README.md#documentation) diff --git a/docs/recipes/rotate-many-keys.md b/docs/recipes/rotate-many-keys.md new file mode 100644 index 0000000..dc0d7e5 --- /dev/null +++ b/docs/recipes/rotate-many-keys.md @@ -0,0 +1,39 @@ +# Rotate or repair many keys at once + +Rotate a whole category, a profile or every key in one command, and rebuild +public keys that went missing or wrong. + +See what a batch would do before it does it: + +```sh +sshmgr rotate --all --dry-run +``` + +Rotate by category, profile, host alias or key, in any mix: + +```sh +sshmgr rotate --category clients +sshmgr rotate simtabi github-imani +``` + +The batch asks once, writes one backup, then rotates each key with the usual +stage, deploy, verify and commit. A key that cannot be verified is left exactly +as it was and named at the end; the others still rotate. + +To rebuild `.pub` files from the keys they belong to (from the private key, or +from the vault item for a 1Password profile): + +```sh +sshmgr key regen-pub --all --dry-run +sshmgr key regen-pub --all +``` + +A `.pub` reported as `rewritten` was a different key, so redeploy anything you +set up from it: `sshmgr deploy `. + +Full reference: [`rotate`](../tools/rotate.md#rotating-a-batch) · +[`key regen-pub`](../tools/validate.md). + +--- + +[← Docs index](../../README.md#documentation) diff --git a/docs/tools/doctor.md b/docs/tools/doctor.md index a63c396..a8b5464 100644 --- a/docs/tools/doctor.md +++ b/docs/tools/doctor.md @@ -20,6 +20,7 @@ sshmgr doctor --strict # every dangling-key state is fatal (CI) | `home` / `ssh` | the resolved config home and `~/.ssh`, so an override is visible | | `providers` | whether the catalog is the shipped default or a user file | | `agent` | whether `ssh-agent` is running and how many keys it holds | +| `1Password` | when a profile uses `key_store: 1password`: that `op` is on PATH, the agent socket exists, and the agent serves the profile's vault (`agent.toml`) | | `known_hosts` | whether the trust store exists, and which hosts are unpinned | | `config drift` | whether the rendered block still matches the manifest | | `perms` | every managed path whose mode is not what it should be | diff --git a/docs/tools/rotate.md b/docs/tools/rotate.md index ca2dd39..7fbcf05 100644 --- a/docs/tools/rotate.md +++ b/docs/tools/rotate.md @@ -2,6 +2,7 @@ ```sh sshmgr rotate [--allow-unverified] [--passphrase] [--yes] +sshmgr rotate [|| ...] [--category ] [--all] [--dry-run] sshmgr rollback [--yes] ``` @@ -35,6 +36,27 @@ with a token) verify by logging in / `ssh -T`. A **manual / web-panel** target (e.g. ploi) can't auto-verify, so rotation **aborts by default** - finish the paste in the panel and re-run with `--allow-unverified` to accept it. +## Rotating a batch + +`rotate` takes any number of keys, profiles or host aliases, every key in a +category (`--category orgs`), or every key (`--all`): + +```sh +sshmgr rotate --all --dry-run # list what would rotate +sshmgr rotate --category clients +sshmgr rotate simtabi imanimanyara/imanimanyara_github-ed25519 +``` + +A batch asks once, writes one encrypted backup and one snapshot, then rotates +each key on its own with the flow above. A key that fails (an unreachable host, +a target that will not verify) is left exactly as it was, and the rest still +rotate; the command exits non-zero and names the keys it left. The inventory is +saved after every committed key, so an interrupted batch keeps what it +finished. A key no host uses is skipped: there is nothing to deploy it to. + +A selector that matches nothing is an error, so a typo never runs as an empty +batch, and `--all` cannot be combined with other selectors. + ## Shared keys For a `shared` profile key the deploy/verify/revoke steps **fan out to every host** @@ -48,6 +70,39 @@ available until the next rotation supersedes it. `doctor` asserts the ≤1-old invariant (counted per key name). +## Keys kept in 1Password + +For a profile with `key_store: 1password` the flow is the same, with the vault +standing in for the private key files: + +1. **Stage** a new SSH Key item titled `.staged` in the profile's vault + (a leftover from a crashed rotation is archived first); its public half goes + to `.staging/`. +2. **Deploy** and **verify** as above. A verify login uses the staged `.pub` + through the 1Password agent, so nothing private is read. +3. **Commit:** any earlier `.old` item goes to the vault's Archive (one + predecessor, as on disk), the live item is retitled `.old`, the staged + item takes ``, and the public keys move as files do. A failed step puts + back what had moved. + +`rollback` retitles `.old` back to `` and archives the rotated-in +item. sshmgr archives items and never deletes them, so every one is still +recoverable from 1Password's Archive. + +Rotating a key whose profile has just been switched to `1password` moves it +into the vault: the outgoing key file pair is archived to `old/`, and the new +key exists only as an item. A rollback across that move is refused, because its +predecessor is a key file; set `key_store` back to `file` to roll back. + +## A GitHub machine user + +A key on a GitHub account is deployed, verified and revoked through that +account's API, with the token named by the host's `token_env`. When both the +API and an ssh login are available, verify also checks that GitHub greets the +key as the account the token belongs to, so a token for the wrong account +fails the rotation instead of committing a key the alias cannot push with. See +[Use a GitHub machine user](../recipes/github-machine-user.md). + --- [← Docs index](../../README.md#documentation) diff --git a/docs/tools/validate.md b/docs/tools/validate.md index 6aca8cf..34d556c 100644 --- a/docs/tools/validate.md +++ b/docs/tools/validate.md @@ -21,6 +21,12 @@ For each key it checks: **Encrypted private keys** are *noted, not failed*: the pair can't be verified without the passphrase, but the key is valid. +**Keys in 1Password** (`key_store: 1password`) have no private key on disk. For +them validate checks that the item exists in the profile's vault and that its +public key is the `.pub` on disk. It fails a private key *file* found beside +one: the `.pub` makes it look present, but ssh asks the 1Password agent, which +does not hold it. Without `op` the vault check is noted, not failed. + ```sh sshmgr validate # all keys sshmgr validate work # one profile @@ -30,6 +36,29 @@ sshmgr validate work_hpc-ed25519 # one key Example: a corrupted `.pub`, a mismatched pair, or loose perms each surface as a red ✗ with the specific issue, and the command exits non-zero. +## `sshmgr key regen-pub [selector ...] [--category ] [--all]` + +Rebuilds public keys from the key they belong to, when a `.pub` is missing or +wrong: from the private key with `ssh-keygen -y`, or for a profile with +`key_store: 1password` from its vault item. Selectors work as for `rotate`. + +| Result | Meaning | +|---|---| +| `unchanged` | the `.pub` already matched its key | +| `created` | the `.pub` was missing | +| `rewritten` | the `.pub` was a different key; redeploy anything set up from it | +| `skipped` | the private key has a passphrase (the command to derive it by hand is printed), or is missing | +| `failed` | the key could not be read | + +The old `.pub`'s comment is kept, and the inventory record moves to the +regenerated fingerprint. It never creates or changes a private key: a missing +private key is `reconcile`'s job, a new one `rotate`'s. + +```sh +sshmgr key regen-pub --all --dry-run +sshmgr key regen-pub work +``` + ## `sshmgr providers` Lists every provider in the active catalog - your `/providers.json` if you diff --git a/src/internal/cli/diff.go b/src/internal/cli/diff.go index 7b9955a..da5f31f 100644 --- a/src/internal/cli/diff.go +++ b/src/internal/cli/diff.go @@ -47,8 +47,11 @@ func newDiffCmd() *cobra.Command { continue // hosts sharing one key are one key to mint } seen[ref] = true - priv := m.KeyPath(p.SSHDir, ref) - if fs.Exists(priv) { + onDisk := m.KeyPath(p.SSHDir, ref) + if m.Profiles[ref.Profile].InOnePassword() { + onDisk += ".pub" // the private half lives in 1Password + } + if fs.Exists(onDisk) { present++ } else { missing = append(missing, fmt.Sprintf(" MINT %s (manifest wants it; not on disk)", ref)) diff --git a/src/internal/cli/key.go b/src/internal/cli/key.go index 0c4462a..2f7de64 100644 --- a/src/internal/cli/key.go +++ b/src/internal/cli/key.go @@ -15,6 +15,7 @@ import ( "github.com/simtabi/ssh-manager/src/v3/internal/services/keyaudit" "github.com/simtabi/ssh-manager/src/v3/internal/services/keysvc" "github.com/simtabi/ssh-manager/src/v3/internal/services/lifecycle" + "github.com/simtabi/ssh-manager/src/v3/internal/services/pubkeys" "github.com/simtabi/ssh-manager/src/v3/internal/services/reconciler" ) @@ -26,6 +27,7 @@ func newKeyCmd() *cobra.Command { cmd.AddCommand(newKeyAddCmd()) cmd.AddCommand(newKeyListCmd()) cmd.AddCommand(newKeyDeleteCmd()) + cmd.AddCommand(newKeyRegenPubCmd()) return cmd } @@ -216,3 +218,63 @@ func dash(s string) string { } return s } + +func newKeyRegenPubCmd() *cobra.Command { + var all, dryRun, yes bool + var category string + cmd := &cobra.Command{ + Use: "regen-pub [<[profile/]key>|profile|alias ...] [--category ] [--all]", + Short: "Rebuild public keys from their private key or 1Password item", + Long: "Rebuild each selected key's .pub from the key it belongs to: the private key\n" + + "file (ssh-keygen -y), or the vault item for a profile whose key_store is\n" + + "1password. Repairs a missing or wrong .pub; never changes a private key.\n\n" + + "A .pub that was a different key is reported as rewritten, with a reminder to\n" + + "redeploy. A private key with a passphrase is skipped, with the command to\n" + + "derive it by hand.", + RunE: func(c *cobra.Command, args []string) error { + p, err := resolvePaths(c) + if err != nil { + return err + } + m, err := manifest.Load(p.Manifest()) + if err != nil { + return err + } + inv, err := inventory.Load(p.Inventory()) + if err != nil { + return err + } + rows, err := keysvc.New(m, inv, p.SSHDir).Select(keysvc.Selection{Selectors: args, Category: category, All: all}) + if err != nil { + return err + } + svc := pubkeys.New(m, inv) + if !dryRun { + if err := confirmChange(c, fmt.Sprintf("Regenerate the .pub of %d key(s).", len(rows)), yes); err != nil { + return err + } + snapshotBeforeMutation(p) + } + results := svc.Regenerate(rows, dryRun) + _, _ = fmt.Fprintln(c.OutOrStdout(), pubkeys.Format(results, dryRun)) + failed := false + for _, r := range results { + failed = failed || r.Action == pubkeys.Failed + } + if !dryRun { + if err := inv.Save(p.Inventory()); err != nil { + return err + } + } + if failed { + return errNotClean + } + return nil + }, + } + cmd.Flags().BoolVar(&all, "all", false, "every key in the manifest") + cmd.Flags().StringVar(&category, "category", "", "every key in profiles of this category") + cmd.Flags().BoolVar(&dryRun, "dry-run", false, "report what would change, and change nothing") + cmd.Flags().BoolVarP(&yes, "yes", "y", false, "skip the confirmation prompt") + return cmd +} diff --git a/src/internal/cli/mutate_test.go b/src/internal/cli/mutate_test.go index b5eb8f6..a0b06d1 100644 --- a/src/internal/cli/mutate_test.go +++ b/src/internal/cli/mutate_test.go @@ -517,3 +517,64 @@ func TestProfileEditCategoryRendersTheCategoryFile(t *testing.T) { t.Errorf("profile add --category: category = %q", got) } } + +// A batch rotation: one confirmation, every selected key rotated on its own, a +// key no host uses skipped, and --dry-run touching nothing. +func TestRotateAllRotatesEveryWiredKey(t *testing.T) { + if _, err := exec.LookPath("ssh-keygen"); err != nil { + t.Skip("ssh-keygen not on PATH") + } + p := editHome(t) + t.Setenv("SSH_MANAGER_AGE_RECIPIENT", "") + run(t, "profile", "add", "work") + run(t, "host", "add", "work", "panel-a", "-H", "ploi-a.example", "-u", "x", "--provider", "ploi") + run(t, "host", "add", "work", "panel-b", "-H", "ploi-b.example", "-u", "x", "--provider", "ploi") + run(t, "key", "add", "work", "work_spare-ed25519") // declared, wired to nothing + run(t, "reconcile", "--yes") + + pubOf := func(name string) string { + b, _ := os.ReadFile(filepath.Join(p.SSHDir, "profiles", "work", name+".pub")) + return string(b) + } + before := pubOf("work_panel-a-ed25519") + pubOf("work_panel-b-ed25519") + + dry := run(t, "rotate", "--all", "--dry-run") + if !strings.Contains(dry, "2 key(s) would rotate") || !strings.Contains(dry, "work_spare-ed25519 - no host uses it") { + t.Fatalf("dry run output:\n%s", dry) + } + if pubOf("work_panel-a-ed25519")+pubOf("work_panel-b-ed25519") != before { + t.Fatal("--dry-run changed a key") + } + + out := run(t, "rotate", "--all", "--allow-unverified", "--yes", "--no-key-backup") + if !strings.Contains(out, "2 of 2 key(s) rotated") { + t.Fatalf("rotate --all output:\n%s", out) + } + if pubOf("work_panel-a-ed25519") == "" || strings.Contains(before, pubOf("work_panel-a-ed25519")) || + strings.Contains(before, pubOf("work_panel-b-ed25519")) { + t.Error("not every selected key was rotated") + } +} + +func TestKeyRegenPubRestoresAMissingPublicKey(t *testing.T) { + if _, err := exec.LookPath("ssh-keygen"); err != nil { + t.Skip("ssh-keygen not on PATH") + } + p := editHome(t) + run(t, "profile", "add", "work") + run(t, "host", "add", "work", "gh", "-H", "github.com", "-u", "git") + run(t, "reconcile", "--yes") + pub := filepath.Join(p.SSHDir, "profiles", "work", "work_gh-ed25519.pub") + want, _ := os.ReadFile(pub) + if err := os.Remove(pub); err != nil { + t.Fatal(err) + } + out := run(t, "key", "regen-pub", "work", "--yes") + if !strings.Contains(out, "created work/work_gh-ed25519") { + t.Fatalf("output:\n%s", out) + } + got, _ := os.ReadFile(pub) + if strings.Fields(string(got))[1] != strings.Fields(string(want))[1] { + t.Error("the regenerated .pub is not the key's public half") + } +} diff --git a/src/internal/cli/rotate.go b/src/internal/cli/rotate.go index 122e79b..9c66352 100644 --- a/src/internal/cli/rotate.go +++ b/src/internal/cli/rotate.go @@ -2,68 +2,36 @@ package cli import ( "fmt" + "strings" "github.com/spf13/cobra" "github.com/simtabi/ssh-manager/src/v3/internal/core/inventory" "github.com/simtabi/ssh-manager/src/v3/internal/core/manifest" + "github.com/simtabi/ssh-manager/src/v3/internal/services/keysvc" "github.com/simtabi/ssh-manager/src/v3/internal/services/rotator" ) func newRotateCmd() *cobra.Command { - var allowUnverified, passphrase, yes, noKeyBackup bool + var allowUnverified, passphrase, yes, noKeyBackup, all, dryRun bool + var category string cmd := &cobra.Command{ - Use: "rotate <[profile/]key>", + Use: "rotate [<[profile/]key>|profile|alias ...] [--category ] [--all]", Short: "Zero-downtime staged key rotation", Long: "Zero-downtime staged key rotation.\n\n" + "The key being replaced is archived to the profile's old/ dir, but only one\n" + "predecessor is kept - so rotating again discards the one before it for good.\n" + - "An encrypted bundle is written first unless --no-key-backup is given.", - Args: cobra.ExactArgs(1), + "An encrypted bundle is written first unless --no-key-backup is given.\n\n" + + "Name one key, several, whole profiles or host aliases, every key in a\n" + + "category, or --all. Each key rotates on its own: one that fails is left\n" + + "untouched and the rest still rotate. A key no host uses is skipped, since\n" + + "there is nothing to deploy it to.", RunE: func(c *cobra.Command, args []string) error { - key := args[0] - if err := confirmOrAbort(c, fmt.Sprintf("Rotate %s? (the current key moves to old/, "+ - "replacing any earlier predecessor)", key), yes); err != nil { - return err - } - p, err := resolvePaths(c) - if err != nil { - return err - } - if err := backupKeysBeforeDestroying(p, c.OutOrStdout(), noKeyBackup); err != nil { - return err + if len(args) == 1 && !all && category == "" && !dryRun { + return rotateOne(c, args[0], allowUnverified, passphrase, yes, noKeyBackup) } - m, err := manifest.Load(p.Manifest()) - if err != nil { - return err - } - inv, err := inventory.Load(p.Inventory()) - if err != nil { - return err - } - pw := "" - if passphrase { - secret, err := readPassphrase(c.InOrStdin()) - if err != nil { - return err - } - pw = secret - } - snapshotBeforeMutation(p) - report, err := rotator.New(p, m, inv).Rotate(key, allowUnverified, pw) - if err != nil { - return err - } - if report.Committed { - if err := inv.Save(p.Inventory()); err != nil { - return err - } - } - _, _ = fmt.Fprintln(c.OutOrStdout(), report.Format()) - if !report.Committed { - return errNotClean - } - return nil + return rotateMany(c, keysvc.Selection{Selectors: args, Category: category, All: all}, + allowUnverified, passphrase, yes, noKeyBackup, dryRun) }, } cmd.Flags().BoolVar(&allowUnverified, "allow-unverified", false, "commit even if a target can't auto-verify") @@ -71,9 +39,145 @@ func newRotateCmd() *cobra.Command { cmd.Flags().BoolVarP(&yes, "yes", "y", false, "skip the confirmation prompt") cmd.Flags().BoolVar(&noKeyBackup, "no-key-backup", false, "rotate without writing an encrypted backup first (the earlier predecessor is lost)") + cmd.Flags().BoolVar(&all, "all", false, "rotate every key a host uses") + cmd.Flags().StringVar(&category, "category", "", "rotate every key in profiles of this category") + cmd.Flags().BoolVar(&dryRun, "dry-run", false, "list the keys that would rotate, and change nothing") return cmd } +// rotateOne is the single-key rotation, unchanged from before batches existed. +func rotateOne(c *cobra.Command, key string, allowUnverified, passphrase, yes, noKeyBackup bool) error { + if err := confirmOrAbort(c, fmt.Sprintf("Rotate %s? (the current key moves to old/, "+ + "replacing any earlier predecessor)", key), yes); err != nil { + return err + } + p, err := resolvePaths(c) + if err != nil { + return err + } + if err := backupKeysBeforeDestroying(p, c.OutOrStdout(), noKeyBackup); err != nil { + return err + } + m, err := manifest.Load(p.Manifest()) + if err != nil { + return err + } + inv, err := inventory.Load(p.Inventory()) + if err != nil { + return err + } + pw := "" + if passphrase { + secret, err := readPassphrase(c.InOrStdin()) + if err != nil { + return err + } + pw = secret + } + snapshotBeforeMutation(p) + report, err := rotator.New(p, m, inv).Rotate(key, allowUnverified, pw) + if err != nil { + return err + } + if report.Committed { + if err := inv.Save(p.Inventory()); err != nil { + return err + } + } + _, _ = fmt.Fprintln(c.OutOrStdout(), report.Format()) + if !report.Committed { + return errNotClean + } + return nil +} + +// rotateMany rotates a selection of keys one after another: one confirmation, +// one backup, one snapshot, and the inventory saved after every committed key +// so an interrupted batch keeps what it finished. +func rotateMany(c *cobra.Command, sel keysvc.Selection, allowUnverified, passphrase, yes, noKeyBackup, dryRun bool) error { + out := c.OutOrStdout() + p, err := resolvePaths(c) + if err != nil { + return err + } + m, err := manifest.Load(p.Manifest()) + if err != nil { + return err + } + inv, err := inventory.Load(p.Inventory()) + if err != nil { + return err + } + rows, err := keysvc.New(m, inv, p.SSHDir).Select(sel) + if err != nil { + return err + } + var todo []keysvc.Row + for _, r := range rows { + if !r.Wired() { + _, _ = fmt.Fprintf(out, "skip %s - no host uses it, so there is nothing to rotate it on\n", r.Ref) + continue + } + todo = append(todo, r) + } + if len(todo) == 0 { + _, _ = fmt.Fprintln(out, "nothing to rotate") + return nil + } + names := make([]string, len(todo)) + for i, r := range todo { + names[i] = r.Ref.String() + } + if dryRun { + for _, n := range names { + _, _ = fmt.Fprintln(out, "rotate "+n) + } + _, _ = fmt.Fprintf(out, "(dry run) %d key(s) would rotate\n", len(names)) + return nil + } + if err := confirmOrAbort(c, fmt.Sprintf("Rotate %d key(s): %s? (each current key moves to old/, "+ + "replacing any earlier predecessor)", len(names), strings.Join(names, ", ")), yes); err != nil { + return err + } + if err := backupKeysBeforeDestroying(p, out, noKeyBackup); err != nil { + return err + } + pw := "" + if passphrase { + secret, err := readPassphrase(c.InOrStdin()) + if err != nil { + return err + } + pw = secret + } + snapshotBeforeMutation(p) + rot := rotator.New(p, m, inv) + var failed []string + for _, n := range names { + report, err := rot.Rotate(n, allowUnverified, pw) + switch { + case err != nil: + failed = append(failed, n) + _, _ = fmt.Fprintf(out, "rotation FAILED: %s\n %v\n", n, err) + continue + case report.Committed: + if err := inv.Save(p.Inventory()); err != nil { + return err + } + default: + failed = append(failed, n) + } + _, _ = fmt.Fprintln(out, report.Format()) + } + _, _ = fmt.Fprintf(out, "%d of %d key(s) rotated", len(names)-len(failed), len(names)) + if len(failed) > 0 { + _, _ = fmt.Fprintf(out, "; not rotated (left as they were): %s\n", strings.Join(failed, ", ")) + return errNotClean + } + _, _ = fmt.Fprintln(out) + return nil +} + func newRollbackCmd() *cobra.Command { var yes bool cmd := &cobra.Command{ diff --git a/src/internal/cli/show.go b/src/internal/cli/show.go index 48447b9..e20cb4e 100644 --- a/src/internal/cli/show.go +++ b/src/internal/cli/show.go @@ -178,6 +178,10 @@ func showKey(out io.Writer, keys *keysvc.Service, ref manifest.KeyRef, indent st {d.PrivatePath, d.PrivateOnDisk, uint32(d.PrivateMode)}, {d.PublicPath, d.PublicOnDisk, uint32(d.PublicMode)}, } { + if f.path == d.PrivatePath && d.InOnePassword() && !f.onDisk { + line(" private key in 1Password, vault %s (never on disk)", d.Vault) + continue + } if !f.onDisk { line(" %s (absent)", f.path) continue diff --git a/src/internal/cli/surface_test.go b/src/internal/cli/surface_test.go index bee095e..aaf2b83 100644 --- a/src/internal/cli/surface_test.go +++ b/src/internal/cli/surface_test.go @@ -100,6 +100,17 @@ var goOnly = map[string]string{ // Categories group profiles under profiles// with one Include per // category. v1 had only the flat layout. + // Bulk key work: rotate a selection instead of one key at a time, and + // rebuild public keys from their private key or 1Password item. + "rotate --all": "bulk rotation - every key a host uses", + "rotate --category": "bulk rotation - every key in a category's profiles", + "rotate --dry-run": "bulk rotation - list the batch, change nothing", + "key regen-pub": "rebuild .pub files from the private key or the 1Password item", + "key regen-pub --all": "regen-pub - every key", + "key regen-pub --category": "regen-pub - every key in a category", + "key regen-pub --dry-run": "regen-pub - report only", + "key regen-pub --yes": "confirm-before-changing", + "profile add --category": "categories - file a new profile under profiles//", "profile edit --category": "categories - move a profile's keys and records under profiles//", diff --git a/src/internal/core/manifest/category_test.go b/src/internal/core/manifest/category_test.go index ee05df8..cd48a62 100644 --- a/src/internal/core/manifest/category_test.go +++ b/src/internal/core/manifest/category_test.go @@ -130,3 +130,72 @@ func TestGlobProfileTreeSeesBothLayoutsAndNotArchives(t *testing.T) { t.Errorf("old = %s, want %s", got, want) } } + +func TestKeyStoreValidation(t *testing.T) { + bad := map[string]string{ + "unknown store": `{"version":1,"profiles":{"p":{"key_store":"vault","hosts":[]}}}`, + "1password, no vault": `{"version":1,"profiles":{"p":{"key_store":"1password","hosts":[]}}}`, + "vault without store": `{"version":1,"profiles":{"p":{"op_vault":"Simtabi","hosts":[]}}}`, + } + for name, doc := range bad { + if _, err := loadJSON(t, doc); err == nil { + t.Errorf("%s: loaded, want a validation error", name) + } + } + m, err := loadJSON(t, `{"version":1,"profiles":{"bot":{"key_store":"1password","op_vault":"Simtabi","hosts":[]},"f":{"hosts":[]}}}`) + if err != nil { + t.Fatal(err) + } + if !m.Profiles["bot"].InOnePassword() || m.Profiles["f"].InOnePassword() || !UsesOnePassword(m) { + t.Error("InOnePassword / UsesOnePassword disagree with the manifest") + } + if got := m.IdentityFile("bot", "k"); got != "~/.ssh/profiles/bot/k" { + t.Errorf("IdentityFile = %q: the key's identity must not change with its store", got) + } +} + +func TestOnePasswordAgentSocketPerPlatform(t *testing.T) { + cases := map[string]string{ + "darwin": "~/Library/Group Containers/2BUA8C4S2C.com.1password/t/agent.sock", + "linux": "~/.1password/agent.sock", + "freebsd": "~/.1password/agent.sock", + "windows": "", + } + for goos, want := range cases { + if got := onePasswordAgentFor(goos); got != want { + t.Errorf("%s: %q, want %q", goos, got, want) + } + } + m, _ := loadJSON(t, `{"version":1,"defaults":{"onepassword_agent":"~/.1password/agent.sock"},"profiles":{}}`) + if got := m.Defaults.OnePasswordAgentSocket(); got != "~/.1password/agent.sock" { + t.Errorf("override ignored: %q", got) + } +} + +func TestOnePasswordKeysMustBeDistinctAndEd25519(t *testing.T) { + shared := `{"version":1,"profiles":{ + "a":{"key_store":"1password","op_vault":"Simtabi","hosts":[{"alias":"x","hostname":"h","user":"u","key_name":"bot_github-ed25519"}]}, + "b":{"key_store":"1password","op_vault":"simtabi","hosts":[{"alias":"y","hostname":"h","user":"u","key_name":"bot_github-ed25519"}]}}}` + if _, err := loadJSON(t, shared); err == nil || !strings.Contains(err.Error(), "share one key") { + t.Errorf("same key name, same vault: err = %v", err) + } + // Different vaults, or one side on disk, keep them apart. + for _, ok := range []string{ + strings.Replace(shared, `"op_vault":"simtabi"`, `"op_vault":"Other"`, 1), + strings.Replace(shared, `"b":{"key_store":"1password","op_vault":"simtabi",`, `"b":{`, 1), + } { + if _, err := loadJSON(t, ok); err != nil { + t.Errorf("should load: %v", err) + } + } + rsa := `{"version":1,"defaults":{"key_type":"rsa"},"profiles":{ + "a":{"key_store":"1password","op_vault":"V","hosts":[{"alias":"x","hostname":"h","user":"u","key_name":"a_x-rsa"}]}}}` + if _, err := loadJSON(t, rsa); err == nil || !strings.Contains(err.Error(), "ed25519") { + t.Errorf("rsa default on a 1Password profile: err = %v", err) + } + sk := `{"version":1,"profiles":{"a":{"key_store":"1password","op_vault":"V", + "keys":[{"name":"a_hw-ed25519-sk","type":"ed25519-sk"}],"hosts":[]}}}` + if _, err := loadJSON(t, sk); err == nil { + t.Error("a declared ed25519-sk key on a 1Password profile should be rejected") + } +} diff --git a/src/internal/core/manifest/manifest.go b/src/internal/core/manifest/manifest.go index ea62610..d4f5f2c 100644 --- a/src/internal/core/manifest/manifest.go +++ b/src/internal/core/manifest/manifest.go @@ -12,6 +12,7 @@ import ( "os" "path/filepath" "regexp" + "runtime" "sort" "strings" "unicode" @@ -235,8 +236,14 @@ func (k *KeySpec) UnmarshalJSON(b []byte) error { // Host blocks out of the root config into profiles//config, which the // root config Includes. Empty keeps the flat layout and inline rendering, so a // manifest written before categories existed behaves exactly as it did. +// +// KeyStore says where the profile's private keys live: "file" (the default, +// written as an omitted field) keeps them under ~/.ssh; "1password" keeps them +// as SSH Key items in OPVault, with only the public half on disk. type Profile struct { Category string `json:"category,omitempty"` + KeyStore string `json:"key_store,omitempty"` + OPVault string `json:"op_vault,omitempty"` KeyScope string `json:"key_scope"` KeyName *string `json:"key_name,omitempty"` Keys []KeySpec `json:"keys,omitempty"` @@ -261,6 +268,8 @@ func (p *Profile) UnmarshalJSON(b []byte) error { func (p Profile) MarshalJSON() ([]byte, error) { type wire struct { Category string `json:"category,omitempty"` + KeyStore string `json:"key_store,omitempty"` + OPVault string `json:"op_vault,omitempty"` KeyScope string `json:"key_scope"` KeyName *string `json:"key_name"` Keys []KeySpec `json:"keys,omitempty"` @@ -270,7 +279,7 @@ func (p Profile) MarshalJSON() ([]byte, error) { if hosts == nil { hosts = []Host{} } - return json.Marshal(wire{Category: p.Category, KeyScope: p.KeyScope, KeyName: p.KeyName, Keys: p.Keys, Hosts: hosts}) + return json.Marshal(wire{Category: p.Category, KeyStore: p.KeyStore, OPVault: p.OPVault, KeyScope: p.KeyScope, KeyName: p.KeyName, Keys: p.Keys, Hosts: hosts}) } // ExpiryCheck is the notifier policy. @@ -302,6 +311,10 @@ type Defaults struct { WarnBeforeDays []int `json:"warn_before_days"` ExpiryCheck ExpiryCheck `json:"expiry_check"` GlobalOptions OrderedOptions `json:"global_options"` + // OnePasswordAgent overrides the 1Password SSH agent socket a profile with + // key_store "1password" renders as its IdentityAgent. Empty = the + // platform's standard path (see OnePasswordAgentSocket). + OnePasswordAgent string `json:"onepassword_agent,omitempty"` } func newDefaults() Defaults { @@ -326,12 +339,13 @@ func (d *Defaults) UnmarshalJSON(b []byte) error { // as {} via OrderedOptions, in declaration order. func (d Defaults) MarshalJSON() ([]byte, error) { type wire struct { - KeyType string `json:"key_type"` - KeyScope string `json:"key_scope"` - RotateAfterDays int `json:"rotate_after_days"` - WarnBeforeDays []int `json:"warn_before_days"` - ExpiryCheck ExpiryCheck `json:"expiry_check"` - GlobalOptions OrderedOptions `json:"global_options"` + KeyType string `json:"key_type"` + KeyScope string `json:"key_scope"` + RotateAfterDays int `json:"rotate_after_days"` + WarnBeforeDays []int `json:"warn_before_days"` + ExpiryCheck ExpiryCheck `json:"expiry_check"` + GlobalOptions OrderedOptions `json:"global_options"` + OnePasswordAgent string `json:"onepassword_agent,omitempty"` } warn := d.WarnBeforeDays if warn == nil { @@ -340,6 +354,7 @@ func (d Defaults) MarshalJSON() ([]byte, error) { return json.Marshal(wire{ KeyType: d.KeyType, KeyScope: d.KeyScope, RotateAfterDays: d.RotateAfterDays, WarnBeforeDays: warn, ExpiryCheck: d.ExpiryCheck, GlobalOptions: d.GlobalOptions, + OnePasswordAgent: d.OnePasswordAgent, }) } @@ -626,6 +641,113 @@ func checkCategories(m *Manifest) error { return nil } +// Key stores. The empty string is the file store, so it never has to be +// written; anything else must be named here, because a typo would otherwise +// silently fall back to looking for private keys on disk. +const ( + KeyStoreFile = "file" + KeyStoreOnePassword = "1password" +) + +// checkKeyStores validates each profile's key_store and the vault it needs. +func checkKeyStores(m *Manifest) error { + if err := rejectControl("onepassword_agent", m.Defaults.OnePasswordAgent); err != nil { + return err + } + for _, name := range m.sortedProfileNames() { + p := m.Profiles[name] + switch p.KeyStore { + case "", KeyStoreFile: + if p.OPVault != "" { + return fmt.Errorf("profile %q sets op_vault but its key_store is not %q", name, KeyStoreOnePassword) + } + case KeyStoreOnePassword: + if strings.TrimSpace(p.OPVault) == "" { + return fmt.Errorf("profile %q uses key_store %q but names no op_vault", name, KeyStoreOnePassword) + } + if err := rejectControl("op_vault", p.OPVault); err != nil { + return err + } + default: + return fmt.Errorf("profile %q: key_store must be %q or %q (got %q)", + name, KeyStoreFile, KeyStoreOnePassword, p.KeyStore) + } + } + return nil +} + +// OnePasswordAgentSocket is the IdentityAgent a 1Password profile renders: the +// defaults override when set, else the platform's standard socket (checked +// against 1password.dev/ssh/get-started, 2026-10-19). "" on Windows, where +// Microsoft OpenSSH talks to one fixed pipe and per-host agents do not apply. +func (d Defaults) OnePasswordAgentSocket() string { + if d.OnePasswordAgent != "" { + return d.OnePasswordAgent + } + return onePasswordAgentFor(runtime.GOOS) +} + +func onePasswordAgentFor(goos string) string { + switch goos { + case "darwin": + return "~/Library/Group Containers/2BUA8C4S2C.com.1password/t/agent.sock" + case "windows": + return "" + default: + return "~/.1password/agent.sock" + } +} + +// checkOnePasswordKeys rejects two key setups that 1Password cannot hold +// apart or cannot make. It runs after the per-profile checks, because it +// resolves every profile's key names. +// +// - A vault item is titled with the bare key name, and key names are unique +// only within a profile. Two 1Password profiles sharing a vault and a key +// name would find each other's item: two identities, one key. +// - 1Password generates Ed25519 keys only, so any other key_type on a +// 1Password profile would be silently ignored. +func checkOnePasswordKeys(m *Manifest) error { + refs, err := m.KeyRefs() + if err != nil { + return err + } + owner := map[string]string{} // vault\x00key -> profile + for _, ref := range refs { + p := m.Profiles[ref.Profile] + if !p.InOnePassword() { + continue + } + if kt := m.KeyTypeFor(ref); kt != "ed25519" { + return fmt.Errorf("key %s is kept in 1Password, which generates ed25519 keys only, "+ + "but its key_type is %q", ref, kt) + } + k := strings.ToLower(p.OPVault) + "\x00" + ref.KeyName + if other, ok := owner[k]; ok && other != ref.Profile { + return fmt.Errorf("profiles %q and %q both keep key %q in 1Password vault %q - the vault "+ + "item is titled with the key name, so they would share one key; rename one of them", + other, ref.Profile, ref.KeyName, p.OPVault) + } + owner[k] = ref.Profile + } + return nil +} + +// InOnePassword reports whether the profile keeps its private keys in 1Password. +func (p Profile) InOnePassword() bool { return p.KeyStore == KeyStoreOnePassword } + +// UsesOnePassword reports whether any profile keeps its keys in 1Password. A +// function rather than a method: Manifest carries a method ceiling (see +// TestNoTypeHasBecomeTheFacadeAgain), and this needs nothing private. +func UsesOnePassword(m *Manifest) bool { + for _, p := range m.Profiles { + if p.KeyStore == KeyStoreOnePassword { + return true + } + } + return false +} + // checkDeclaredKeys validates one profile's keys list. Names are path segments // (they become file names under profiles//) and unique within the // profile - not globally, since a profile models an org and the same person's @@ -663,6 +785,9 @@ func (m *Manifest) validate() error { if err := checkCategories(m); err != nil { return err } + if err := checkKeyStores(m); err != nil { + return err + } for name, p := range m.Profiles { if err := safeSegment("profile name", name); err != nil { return err @@ -698,7 +823,7 @@ func (m *Manifest) validate() error { } } } - return nil + return checkOnePasswordKeys(m) } func (m *Manifest) sortedProfileNames() []string { @@ -815,6 +940,9 @@ func (m *Manifest) Categories() []string { } // IdentityFile is the rendered ~ form path for a key (always forward slashes). +// It names the private key. For a 1Password profile the private key is never on +// disk, so the renderer points ssh at its .pub instead; inventory records and +// every other caller keep this one, so a key has the same identity in both stores. func (m *Manifest) IdentityFile(profileName, keyName string) string { return sshToken + "/" + m.ProfileRel(profileName) + "/" + keyName } diff --git a/src/internal/core/providers/adapter.go b/src/internal/core/providers/adapter.go index 00621f2..4d83858 100644 --- a/src/internal/core/providers/adapter.go +++ b/src/internal/core/providers/adapter.go @@ -16,7 +16,21 @@ type Target struct { Port int TokenEnv string // "" == none IdentityPath string // private key for verify; "" == none - KnownHosts string // per-profile known_hosts; "" == none + // IdentityAgent is the agent holding the key when IdentityPath is a public + // key (a key kept in 1Password); "" == the key is a file. + IdentityAgent string + KnownHosts string // per-profile known_hosts; "" == none +} + +// identityOpts are the ssh options that make a verify login use exactly the +// key under test: the file itself, or the agent's copy of it selected by its +// public key. +func (t Target) identityOpts() []string { + opts := []string{"-i", t.IdentityPath, "-o", "IdentitiesOnly=yes"} + if t.IdentityAgent != "" { + opts = append(opts, "-o", "IdentityAgent="+t.IdentityAgent) + } + return opts } // SSHDest is user@hostname. diff --git a/src/internal/core/providers/greeting_test.go b/src/internal/core/providers/greeting_test.go new file mode 100644 index 0000000..f18c9bc --- /dev/null +++ b/src/internal/core/providers/greeting_test.go @@ -0,0 +1,66 @@ +package providers + +import ( + "os" + "path/filepath" + "runtime" + "testing" +) + +func TestGreetedLogin(t *testing.T) { + cases := map[string]string{ + "Hi simtabi-bot! You've successfully authenticated, but GitHub does not provide shell access.": "simtabi-bot", + "Hi imanimanyara! You've successfully authenticated, but GitHub does not provide shell access.": "imanimanyara", + "git@github.com: Permission denied (publickey).": "", + "": "", + } + for banner, want := range cases { + if got := greetedLogin(banner); got != want { + t.Errorf("greetedLogin(%q) = %q, want %q", banner, got, want) + } + } +} + +func TestVerifyLoginUsesTheAgentForAPublicKey(t *testing.T) { + file := Target{IdentityPath: "/k"}.identityOpts() + agent := Target{IdentityPath: "/k.pub", IdentityAgent: "~/.1password/agent.sock"}.identityOpts() + if len(file) != 4 || len(agent) != 6 || agent[5] != "IdentityAgent=~/.1password/agent.sock" { + t.Errorf("file opts %v, agent opts %v", file, agent) + } +} + +// A token for the wrong account (a personal one left in GH_TOKEN_SIMTABI) lists +// the key on that account, so "the key is listed" passes; GitHub then greets the +// key as someone else. Verify must refuse that, or rotation commits a key the +// machine-user alias cannot push with. +func TestGitHubVerifyRejectsAKeyThatAuthenticatesAsAnotherAccount(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("stubs are shell scripts") + } + const pub = "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIBUS2DuYRWw/OWok0ZTrm7v2vmro3+jJ+/n36DZDuitx simtabi-bot" // throwaway, generated for this test + bin := t.TempDir() + write := func(name, body string) { + if err := os.WriteFile(filepath.Join(bin, name), []byte("#!/bin/sh\n"+body), 0o755); err != nil { + t.Fatal(err) + } + } + write("gh", `case "$*" in +*user/keys*) echo '[{"id":1,"key":"`+pub+`","title":"t"}]' ;; +*"api user"*) echo "$FAKE_LOGIN" ;; +esac +`) + write("ssh", `echo "Hi $FAKE_GREETING! You've successfully authenticated, but GitHub does not provide shell access." >&2; exit 1`+"\n") + t.Setenv("PATH", bin+string(os.PathListSeparator)+os.Getenv("PATH")) + t.Setenv("GH_TOKEN_TEST", "tok") + tgt := Target{PubkeyText: pub, TokenEnv: "GH_TOKEN_TEST", IdentityPath: "/k.pub"} + + t.Setenv("FAKE_LOGIN", "simtabi-bot") + t.Setenv("FAKE_GREETING", "simtabi-bot") + if !(GitHub{}).Verify(tgt) { + t.Fatal("matching account: Verify = false, want true") + } + t.Setenv("FAKE_GREETING", "imanimanyara") + if (GitHub{}).Verify(tgt) { + t.Fatal("key authenticates as another account: Verify = true, want false") + } +} diff --git a/src/internal/core/providers/ssh_generic.go b/src/internal/core/providers/ssh_generic.go index 62c899a..fd8a6a1 100644 --- a/src/internal/core/providers/ssh_generic.go +++ b/src/internal/core/providers/ssh_generic.go @@ -125,8 +125,9 @@ func (g GenericSSH) Verify(t Target) bool { if t.IdentityPath == "" { return false } - args := append([]string{"ssh", "-i", t.IdentityPath, "-o", "BatchMode=yes", - "-o", "IdentitiesOnly=yes", "-o", "StrictHostKeyChecking=accept-new"}, g.knownHostsOpts(t)...) + args := append([]string{"ssh"}, t.identityOpts()...) + args = append(args, "-o", "BatchMode=yes", "-o", "StrictHostKeyChecking=accept-new") + args = append(args, g.knownHostsOpts(t)...) if t.Port != 22 { args = append(args, "-p", strconv.Itoa(t.Port)) } diff --git a/src/internal/core/providers/vcs.go b/src/internal/core/providers/vcs.go index 7f0805a..ecb993c 100644 --- a/src/internal/core/providers/vcs.go +++ b/src/internal/core/providers/vcs.go @@ -142,26 +142,74 @@ func (g GitHub) Deploy(t Target) DeployOutcome { return DeployOutcome{Method: "github-gh", Verified: true, Detail: "added as '" + title + "'"} } +// Verify confirms the key is on the account and, where both can be read, that +// GitHub authenticates it as that same account. +// +// The account check matters for a machine user: the key is added with whatever +// token token_env names, and a token for the wrong account (a personal one left +// in GH_TOKEN_SIMTABI) would add it there and still pass a "key is listed" +// check - the alias would then push as the wrong identity. So when the API +// login and the ssh greeting are both available they must agree. Either one +// missing (no network for ssh, no API) falls back to the check that remains. func (g GitHub) Verify(t Target) bool { + greeted := g.sshLogin(t) if g.canAPI(t) { want := authkeys.KeyBody(t.PubkeyText) rows := g.listRemote(t) - return rows != nil && want != "" && hasKeyBody(rows, want) + if rows == nil || want == "" || !hasKeyBody(rows, want) { + return false + } + if login := g.apiLogin(t); greeted != "" && login != "" && !strings.EqualFold(greeted, login) { + return false + } + return true } + return greeted != "" +} + +// sshLogin is the account GitHub greets the key as ("Hi ! You've +// successfully authenticated"), or "" when the login fails or cannot be tried. +func (g GitHub) sshLogin(t Target) string { if t.IdentityPath == "" { - return false + return "" } if _, err := exec.LookPath("ssh"); err != nil { - return false + return "" } - args := []string{"ssh", "-T", "-i", t.IdentityPath, "-o", "IdentitiesOnly=yes", - "-o", "StrictHostKeyChecking=accept-new", "-o", "BatchMode=yes", "-o", "ConnectTimeout=10"} + args := append([]string{"ssh", "-T"}, t.identityOpts()...) + args = append(args, "-o", "StrictHostKeyChecking=accept-new", "-o", "BatchMode=yes", "-o", "ConnectTimeout=10") if t.KnownHosts != "" { args = append(args, "-o", "UserKnownHostsFile="+t.KnownHosts) } args = append(args, "git@"+g.host()) out, stderr, _ := runEnv(20*time.Second, nil, args...) - return strings.Contains(strings.ToLower(stderr+out), "successfully authenticated") + return greetedLogin(stderr + out) +} + +// greetedLogin extracts from GitHub's "Hi ! You've successfully +// authenticated" banner. +func greetedLogin(banner string) string { + if !strings.Contains(strings.ToLower(banner), "successfully authenticated") { + return "" + } + i := strings.Index(banner, "Hi ") + if i < 0 { + return "" + } + rest := banner[i+3:] + if j := strings.IndexAny(rest, "!"); j > 0 { + return strings.TrimSpace(rest[:j]) + } + return "" +} + +// apiLogin is the login the token belongs to, or "". +func (g GitHub) apiLogin(t Target) string { + out, _, code := runEnv(30*time.Second, g.env(t), "gh", "api", "user", "--jq", ".login") + if code != 0 { + return "" + } + return strings.TrimSpace(out) } func (g GitHub) ListDeployed(t Target) []string { diff --git a/src/internal/core/renderer/category_test.go b/src/internal/core/renderer/category_test.go index f9cba19..fa6f5e3 100644 --- a/src/internal/core/renderer/category_test.go +++ b/src/internal/core/renderer/category_test.go @@ -158,3 +158,62 @@ func TestIsManagedFileRecognisesBothHeaders(t *testing.T) { } } } + +const onePasswordJSON = `{ + "version": 1, + "defaults": {"key_type": "ed25519", "onepassword_agent": "~/Library/Group Containers/2BUA8C4S2C.com.1password/t/agent.sock"}, + "profiles": { + "bot": {"key_store": "1password", "op_vault": "Simtabi", "hosts": [ + {"alias": "github-simtabi", "hostname": "github.com", "user": "git", "key_name": "simtabibot_github-ed25519"}]}, + "me": {"hosts": [ + {"alias": "github-imani", "hostname": "github.com", "user": "git", "key_name": "imanimanyara_github-ed25519"}]} + } +}` + +func TestOnePasswordHostsUseThePublicKeyAndTheAgent(t *testing.T) { + m := mustManifest(t, onePasswordJSON) + out, err := RenderAll(m, false) + if err != nil { + t.Fatal(err) + } + root := out[RootConfig] + want := "Host github-simtabi\n HostName github.com\n User git\n" + + " IdentityFile ~/.ssh/profiles/bot/simtabibot_github-ed25519.pub\n" + + " IdentityAgent \"~/Library/Group Containers/2BUA8C4S2C.com.1password/t/agent.sock\"\n" + + " IdentitiesOnly yes\n" + if !strings.Contains(root, want) { + t.Errorf("1Password host block:\n%s\nwant it to contain:\n%s", root, want) + } + file := root[strings.Index(root, "Host github-imani"):] + file = file[:strings.Index(file, "\n\n")] + if strings.Contains(file, "IdentityAgent") || strings.Contains(file, ".pub") { + t.Errorf("a file-store host must not change:\n%s", file) + } + // ssh itself must read the quoted path with its space as one value. + if _, err := exec.LookPath("ssh"); err != nil || runtime.GOOS == "windows" { + return + } + cfg := filepath.Join(t.TempDir(), "config") + if err := os.WriteFile(cfg, []byte(root), 0o600); err != nil { + t.Fatal(err) + } + b, err := exec.Command("ssh", "-G", "-F", cfg, "github-simtabi").CombinedOutput() + if err != nil { + t.Fatalf("ssh -G: %v\n%s", err, b) + } + if !strings.Contains(string(b), "identityagent ") || !strings.Contains(string(b), "2BUA8C4S2C.com.1password/t/agent.sock") { + t.Errorf("ssh did not read the IdentityAgent path:\n%s", b) + } +} + +func TestAnExplicitIdentityAgentWins(t *testing.T) { + m := mustManifest(t, strings.Replace(onePasswordJSON, `"key_name": "simtabibot_github-ed25519"}`, + `"key_name": "simtabibot_github-ed25519", "raw_options": {"IdentityAgent": "~/.other.sock"}}`, 1)) + out, err := RenderAll(m, false) + if err != nil { + t.Fatal(err) + } + if n := strings.Count(out[RootConfig], "IdentityAgent"); n != 1 || !strings.Contains(out[RootConfig], "~/.other.sock") { + t.Errorf("want only the host's own IdentityAgent:\n%s", out[RootConfig]) + } +} diff --git a/src/internal/core/renderer/renderer.go b/src/internal/core/renderer/renderer.go index faadfba..bb770eb 100644 --- a/src/internal/core/renderer/renderer.go +++ b/src/internal/core/renderer/renderer.go @@ -48,8 +48,11 @@ type RenderHost struct { User string Port int IdentityFile string - KnownHosts string - RawOptions manifest.OrderedOptions + // IdentityAgent is the agent socket for a key kept in 1Password, "" for a + // key on disk. Rendered quoted: the macOS socket path contains a space. + IdentityAgent string + KnownHosts string + RawOptions manifest.OrderedOptions } const identitiesOnlyKey = "IdentitiesOnly" @@ -106,6 +109,9 @@ func writeHostBlock(b *strings.Builder, h RenderHost) { b.WriteString(" Port " + strconv.Itoa(h.Port) + "\n") } b.WriteString(" IdentityFile " + h.IdentityFile + "\n") + if h.IdentityAgent != "" && !hasOption(h.RawOptions, "IdentityAgent") { + b.WriteString(" IdentityAgent \"" + h.IdentityAgent + "\"\n") + } b.WriteString(" IdentitiesOnly " + identitiesOnly(h.RawOptions) + "\n") b.WriteString(" UserKnownHostsFile " + h.KnownHosts + "\n") for _, k := range h.RawOptions.Keys() { @@ -117,6 +123,28 @@ func writeHostBlock(b *strings.Builder, h RenderHost) { b.WriteString("\n") // blank line between/after host blocks } +// hasOption reports whether the host sets key itself; an explicit value wins. +func hasOption(opts manifest.OrderedOptions, key string) bool { + for _, k := range opts.Keys() { + if strings.EqualFold(k, key) { + return true + } + } + return false +} + +// identityFor is what a host block points ssh at: the private key on disk, or +// for a 1Password profile the public key plus the 1Password agent, which is +// how ssh picks one key out of an agent. +func identityFor(m *manifest.Manifest, profile, keyName string) (file, agent string) { + file = m.IdentityFile(profile, keyName) + if m.Profiles[profile].InOnePassword() { + file += ".pub" + agent = m.Defaults.OnePasswordAgentSocket() + } + return file, agent +} + func writeGlobalBlock(b *strings.Builder, globalOptions manifest.OrderedOptions, emitUseKeychain bool) { b.WriteString("Host *\n") for _, k := range globalOptions.Keys() { @@ -201,14 +229,16 @@ func hostsByProfile(m *manifest.Manifest) (map[string][]RenderHost, error) { } byProfile := map[string][]RenderHost{} for _, rk := range resolved { + file, agent := identityFor(m, rk.Profile, rk.KeyName) byProfile[rk.Profile] = append(byProfile[rk.Profile], RenderHost{ - Alias: rk.Host.Alias, - Hostname: rk.Host.Hostname, - User: rk.Host.User, - Port: rk.Host.Port, - IdentityFile: rk.IdentityFile, - KnownHosts: m.KnownHostsFile(), - RawOptions: rk.Host.RawOptions, + Alias: rk.Host.Alias, + Hostname: rk.Host.Hostname, + User: rk.Host.User, + Port: rk.Host.Port, + IdentityFile: file, + IdentityAgent: agent, + KnownHosts: m.KnownHostsFile(), + RawOptions: rk.Host.RawOptions, }) } return byProfile, nil @@ -240,15 +270,17 @@ func RenderHostBlockFor(m *manifest.Manifest, profile string, host manifest.Host if err != nil { return "", err } + file, agent := identityFor(m, profile, kname) var b strings.Builder writeHostBlock(&b, RenderHost{ - Alias: host.Alias, - Hostname: host.Hostname, - User: host.User, - Port: host.Port, - IdentityFile: m.IdentityFile(profile, kname), - KnownHosts: m.KnownHostsFile(), - RawOptions: host.RawOptions, + Alias: host.Alias, + Hostname: host.Hostname, + User: host.User, + Port: host.Port, + IdentityFile: file, + IdentityAgent: agent, + KnownHosts: m.KnownHostsFile(), + RawOptions: host.RawOptions, }) return b.String(), nil } diff --git a/src/internal/services/agent/agent.go b/src/internal/services/agent/agent.go index e470539..7ac52fe 100644 --- a/src/internal/services/agent/agent.go +++ b/src/internal/services/agent/agent.go @@ -61,6 +61,9 @@ func Load(m *manifest.Manifest, sshDir, profile string, add func(string) bool) ( if rk.Profile != profile { continue } + if m.Profiles[rk.Profile].InOnePassword() { + continue // the 1Password agent serves it; there is no private key file to add + } priv := m.KeyPath(sshDir, manifest.KeyRef{Profile: rk.Profile, KeyName: rk.KeyName}) if seen[priv] { continue diff --git a/src/internal/services/doctor/doctor.go b/src/internal/services/doctor/doctor.go index 1a0ea72..0ee1ee3 100644 --- a/src/internal/services/doctor/doctor.go +++ b/src/internal/services/doctor/doctor.go @@ -23,6 +23,7 @@ import ( "github.com/simtabi/ssh-manager/src/v3/internal/services/keyaudit" "github.com/simtabi/ssh-manager/src/v3/internal/services/keystore" "github.com/simtabi/ssh-manager/src/v3/internal/services/knownhosts" + "github.com/simtabi/ssh-manager/src/v3/internal/services/opstore" "github.com/simtabi/ssh-manager/src/v3/internal/services/preflight" "github.com/simtabi/ssh-manager/src/v3/internal/util/fs" "github.com/simtabi/ssh-manager/src/v3/internal/util/homeperms" @@ -48,6 +49,9 @@ type Report struct { AliasCollisions []string ProvidersSource string // "user file" | "shipped default" StrandedLegacyHome string + // OnePassword lists what stops a 1Password profile from working: no op + // CLI, or no agent socket. Empty when no profile uses 1Password. + OnePassword []string } // OK is the overall verdict. @@ -57,7 +61,7 @@ type Report struct { // result nothing acts on is not a check. Which states are fatal is keyaudit's // call, not this one's (--strict makes all of them fatal). func (r Report) OK() bool { - if !r.Preflight.OK() || len(r.PermIssues) > 0 || !r.ConfigInSync { + if !r.Preflight.OK() || len(r.PermIssues) > 0 || !r.ConfigInSync || len(r.OnePassword) > 0 { return false } if !r.Dangling.OK() { @@ -89,6 +93,9 @@ func (r Report) Format() string { "the legacy one).", r.StrandedLegacyHome)) } lines = append(lines, "agent: "+r.AgentStatus) + for _, issue := range r.OnePassword { + lines = append(lines, "1Password: "+issue) + } lines = append(lines, "known_hosts: "+presentAbsent(r.KnownHosts)) if len(r.UnpinnedHosts) > 0 { lines = append(lines, "host keys NOT pinned (ssh/git will fail host-key "+ @@ -265,10 +272,50 @@ func (s *Service) Run(strict bool) Report { rep.DuplicateKeys = duplicateKeys(ssh) rep.UnpinnedHosts = s.unpinnedHosts(ssh) rep.AliasCollisions = aliasCollisions(s.m) + rep.OnePassword = onePasswordIssues(s.m) } return rep } +// onePasswordIssues reports what a profile with key_store "1password" needs and +// lacks: the op CLI (to mint, validate and rotate) and the agent socket (which +// ssh is pointed at for every such host). +func onePasswordIssues(m *manifest.Manifest) []string { + if !manifest.UsesOnePassword(m) { + return nil + } + var out []string + if err := opstore.New().Available(); err != nil { + out = append(out, err.Error()) + } + if sock := m.Defaults.OnePasswordAgentSocket(); sock != "" { + path := sock + if strings.HasPrefix(path, "~/") { + if home, err := os.UserHomeDir(); err == nil { + path = filepath.Join(home, path[2:]) + } + } + if _, err := os.Stat(path); err != nil { + out = append(out, "the SSH agent socket "+sock+" is missing - turn on "+ + "Settings > Developer > Use the SSH agent in the 1Password app") + } + } + // A key in a vault the agent does not serve is never offered to ssh, which + // shows up only as "Permission denied (publickey)". + seen := map[string]bool{} + for _, name := range m.ProfileNames() { + p := m.Profiles[name] + if !p.InOnePassword() || seen[strings.ToLower(p.OPVault)] { + continue + } + seen[strings.ToLower(p.OPVault)] = true + if ok, why := opstore.AgentServesVault(p.OPVault, opstore.AgentConfigPath()); !ok { + out = append(out, why) + } + } + return out +} + func (s *Service) providersSource() string { if fs.Exists(s.p.Providers()) { return "user file" diff --git a/src/internal/services/doctor/onepassword_test.go b/src/internal/services/doctor/onepassword_test.go new file mode 100644 index 0000000..e96aa11 --- /dev/null +++ b/src/internal/services/doctor/onepassword_test.go @@ -0,0 +1,24 @@ +package doctor + +import ( + "encoding/json" + "strings" + "testing" + + "github.com/simtabi/ssh-manager/src/v3/internal/core/manifest" +) + +func TestOnePasswordIssuesOnlyWhenAProfileUsesIt(t *testing.T) { + var none, uses manifest.Manifest + _ = json.Unmarshal([]byte(`{"version":1,"profiles":{"p":{"hosts":[]}}}`), &none) + if got := onePasswordIssues(&none); got != nil { + t.Errorf("no 1Password profile: issues = %v, want none", got) + } + _ = json.Unmarshal([]byte(`{"version":1,"defaults":{"onepassword_agent":"~/.no-such-dir-9f2/agent.sock"}, + "profiles":{"bot":{"key_store":"1password","op_vault":"V","hosts":[]}}}`), &uses) + t.Setenv("PATH", t.TempDir()) // no op + got := strings.Join(onePasswordIssues(&uses), "|") + if !strings.Contains(got, "op") || !strings.Contains(got, "socket") { + t.Errorf("issues = %q, want both the missing op and the missing socket", got) + } +} diff --git a/src/internal/services/keyaudit/keyaudit.go b/src/internal/services/keyaudit/keyaudit.go index 0ea0d65..51d3f59 100644 --- a/src/internal/services/keyaudit/keyaudit.go +++ b/src/internal/services/keyaudit/keyaudit.go @@ -160,12 +160,18 @@ func (r Report) Lines() []string { func Notes(row keysvc.Row) []string { var out []string switch { + case row.InOnePassword(): + // Only the public half lives on disk; the private one is in 1Password, + // so its absence is the expected state, not half a pair. + if !row.PublicOnDisk { + out = append(out, Missing) + } case !row.PrivateOnDisk && !row.PublicOnDisk: out = append(out, Missing) case !row.PrivateOnDisk || !row.PublicOnDisk: out = append(out, HalfPair) } - if row.PrivateOnDisk && !row.Recorded { + if (row.PrivateOnDisk || (row.InOnePassword() && row.PublicOnDisk)) && !row.Recorded { out = append(out, Unrecorded) } if !row.Wired() { @@ -237,6 +243,10 @@ func (s *Service) manifestKeyFinding(row keysvc.Row, state string) Finding { switch state { case Missing: f.Detail = "the manifest references it; neither half is on disk" + if row.InOnePassword() { + f.Detail = "the manifest references it; its public key is not on disk " + + "(the private key lives in 1Password vault " + row.Vault + ")" + } f.Fix = "sshmgr reconcile" case HalfPair: half := "the private key is missing" diff --git a/src/internal/services/keyaudit/keyaudit_test.go b/src/internal/services/keyaudit/keyaudit_test.go index f10a36f..c53a4a1 100644 --- a/src/internal/services/keyaudit/keyaudit_test.go +++ b/src/internal/services/keyaudit/keyaudit_test.go @@ -9,6 +9,7 @@ import ( "github.com/simtabi/ssh-manager/src/v3/internal/core/inventory" "github.com/simtabi/ssh-manager/src/v3/internal/core/manifest" + "github.com/simtabi/ssh-manager/src/v3/internal/services/keysvc" ) const manifestJSON = `{ @@ -458,3 +459,20 @@ func TestKeyMovedIntoACategoryByHandIsReportedWhereItIs(t *testing.T) { t.Errorf("the missing detail should say where the key actually is: %q", missing[0].Detail) } } + +// A 1Password key has only its public half on disk. That is the expected +// state, not half a pair, and losing the .pub is what makes it missing. +func TestOnePasswordKeyNeedsOnlyItsPublicHalf(t *testing.T) { + inOP := keysvc.Row{Vault: "Simtabi", PublicOnDisk: true, Recorded: true, Hosts: []string{"gh"}} + if got := Notes(inOP); len(got) != 0 { + t.Errorf("public half present: notes = %v, want none", got) + } + gone := keysvc.Row{Vault: "Simtabi", Recorded: true, Hosts: []string{"gh"}} + if got := Notes(gone); len(got) != 1 || got[0] != Missing { + t.Errorf("public half gone: notes = %v, want [missing]", got) + } + unrecorded := keysvc.Row{Vault: "Simtabi", PublicOnDisk: true, Hosts: []string{"gh"}} + if got := Notes(unrecorded); len(got) != 1 || got[0] != Unrecorded { + t.Errorf("no inventory record: notes = %v, want [unrecorded]", got) + } +} diff --git a/src/internal/services/keysvc/keysvc.go b/src/internal/services/keysvc/keysvc.go index eee8cc7..1e2509f 100644 --- a/src/internal/services/keysvc/keysvc.go +++ b/src/internal/services/keysvc/keysvc.go @@ -14,6 +14,7 @@ package keysvc import ( + "fmt" "os" "sort" @@ -42,8 +43,23 @@ type Row struct { Created string ExpiresOn string Hosts []string // aliases wiring this key, in manifest order - Deployments []inventory.Deployment - Status string // query.NoKey | query.NeedsRedeploy | query.Deployed + // Vault is the 1Password vault holding the private key, "" for a key on + // disk. When set, PrivatePath is never expected to exist. + Vault string + Deployments []inventory.Deployment + Status string // query.NoKey | query.NeedsRedeploy | query.Deployed +} + +// InOnePassword reports whether the private key lives in 1Password. +func (r Row) InOnePassword() bool { return r.Vault != "" } + +// Present reports whether everything the key's store keeps on disk is there: +// both halves for a key on disk, the public half for a key in 1Password. +func (r Row) Present() bool { + if r.InOnePassword() { + return r.PublicOnDisk + } + return r.PrivateOnDisk && r.PublicOnDisk } // Wired reports whether any host references this key. @@ -158,6 +174,9 @@ func (s *Service) row(ref manifest.KeyRef) (Row, error) { Hosts: aliases, Status: query.NoKey, } + if s.m.Profiles[ref.Profile].InOnePassword() { + row.Vault = s.m.Profiles[ref.Profile].OPVault + } if fp, rec, ok := s.record(ident); ok { row.Recorded = true row.Fingerprint = fp @@ -219,3 +238,67 @@ func deref(s *string) string { } return *s } + +// Selection names the keys a bulk command acts on: some selectors (each a +// profile, host alias, key name or profile/key), every key in a category, or +// every key. All excludes the others, so "--all work" cannot quietly mean all. +type Selection struct { + Selectors []string + Category string + All bool +} + +// Select resolves a Selection to rows, de-duplicated, in manifest order. A +// selector or category that matches nothing is an error, so a typo never reads +// as an empty batch, and an empty Selection is an error rather than "nothing". +func (s *Service) Select(sel Selection) ([]Row, error) { + if sel.All && (len(sel.Selectors) > 0 || sel.Category != "") { + return nil, fmt.Errorf("--all selects every key; drop it, or drop the other selectors") + } + if !sel.All && len(sel.Selectors) == 0 && sel.Category == "" { + return nil, fmt.Errorf("name keys, profiles or host aliases, or pass --category or --all") + } + rows, err := s.Rows("") + if err != nil { + return nil, err + } + if sel.All { + return rows, nil + } + if sel.Category != "" && !hasCategory(s.m, sel.Category) { + return nil, fmt.Errorf("no profile has category %q", sel.Category) + } + for _, want := range sel.Selectors { + hit := false + for _, r := range rows { + if matches(want, r) { + hit = true + break + } + } + if !hit { + return nil, fmt.Errorf("unknown key, profile or host: %q", want) + } + } + var out []Row + for _, r := range rows { + inCategory := sel.Category != "" && s.m.Profiles[r.Ref.Profile].Category == sel.Category + picked := inCategory + for _, want := range sel.Selectors { + picked = picked || matches(want, r) + } + if picked { + out = append(out, r) + } + } + return out, nil +} + +func hasCategory(m *manifest.Manifest, category string) bool { + for _, p := range m.Profiles { + if p.Category == category { + return true + } + } + return false +} diff --git a/src/internal/services/keysvc/keysvc_test.go b/src/internal/services/keysvc/keysvc_test.go index 7e34b64..3ad66b1 100644 --- a/src/internal/services/keysvc/keysvc_test.go +++ b/src/internal/services/keysvc/keysvc_test.go @@ -5,6 +5,7 @@ import ( "os" "os/exec" "path/filepath" + "strings" "testing" "github.com/simtabi/ssh-manager/src/v3/internal/core/inventory" @@ -305,3 +306,47 @@ func TestTwoRecordsAtOnePathResolveTheSameWayEveryTime(t *testing.T) { } func ptr(s string) *string { return &s } + +func refsOf(rows []Row) string { + var out []string + for _, r := range rows { + out = append(out, r.Ref.String()) + } + return strings.Join(out, ",") +} + +func TestSelectResolvesABatchAndRejectsWhatItCannot(t *testing.T) { + s := fixture(t) + all, err := s.Select(Selection{All: true}) + if err != nil || len(all) != 3 { + t.Fatalf("--all = %v, %v; want every key", refsOf(all), err) + } + // Overlapping selectors yield each key once, in manifest order. + got, err := s.Select(Selection{Selectors: []string{"gh", "work", "work/work_gh-ed25519"}}) + if err != nil || refsOf(got) != "work/work_gh-ed25519,work/work_spare-rsa" { + t.Errorf("overlapping selectors = %q, %v", refsOf(got), err) + } + for name, sel := range map[string]Selection{ + "empty": {}, + "all plus a selector": {All: true, Selectors: []string{"work"}}, + "a typo": {Selectors: []string{"wrok"}}, + "an unknown category": {Category: "nope"}, + } { + if _, err := s.Select(sel); err == nil { + t.Errorf("%s: want an error, not a batch", name) + } + } +} + +func TestSelectByCategory(t *testing.T) { + var m manifest.Manifest + if err := json.Unmarshal([]byte(`{"version":1,"profiles":{ + "a":{"category":"orgs","hosts":[{"alias":"x","hostname":"h","user":"u","key_name":"a_x-ed25519"}]}, + "b":{"category":"clients","hosts":[{"alias":"y","hostname":"h","user":"u","key_name":"b_y-ed25519"}]}}}`), &m); err != nil { + t.Fatal(err) + } + got, err := New(&m, inventory.New(), t.TempDir()).Select(Selection{Category: "orgs"}) + if err != nil || refsOf(got) != "a/a_x-ed25519" { + t.Errorf("category orgs = %q, %v", refsOf(got), err) + } +} diff --git a/src/internal/services/lifecycle/lifecycle.go b/src/internal/services/lifecycle/lifecycle.go index 902030e..f6dd0b6 100644 --- a/src/internal/services/lifecycle/lifecycle.go +++ b/src/internal/services/lifecycle/lifecycle.go @@ -56,6 +56,9 @@ type Result struct { // UnwiredKeys are keys the deletion left with no host, but which their // profile still declares - so they remain sshmgr's, just unused. UnwiredKeys []string + // VaultItems are 1Password items that held a deleted key. sshmgr never + // deletes a vault item; the user archives it. + VaultItems []string // OrphanedKeys are keys the deletion removed from the manifest entirely, // because nothing declared them and only the deleted host named them. Their // files are still on disk and nothing tracks them any more. @@ -78,6 +81,9 @@ func (r Result) Format() string { lines = append(lines, fmt.Sprintf(" pruned known_hosts: %d line(s) no host needs any more", r.PrunedPins)) } add("re-rendered", r.ConfigWritten) + for _, it := range r.VaultItems { + lines = append(lines, " kept in 1Password: "+it+" - archive it there if it is no longer needed") + } for _, ref := range r.UnwiredKeys { lines = append(lines, fmt.Sprintf(" %s is now UNWIRED: its profile still declares it, but no "+ "host uses it.", ref)) @@ -137,6 +143,10 @@ func (s *Service) DeleteProfile(name string, opt Options) (Result, error) { res := Result{ Removed: "profile " + name, Revoked: del.Revoked, PrunedRecords: del.PrunedKeys, } + if before.Profiles[name].InOnePassword() { + res.VaultItems = append(res.VaultItems, fmt.Sprintf("profile %s's key items in vault %q", + name, before.Profiles[name].OPVault)) + } if err := s.finish(&res, files, before.ProfileDir(s.p.SSHDir, name), opt); err != nil { return res, err } @@ -273,6 +283,9 @@ func (s *Service) DeleteKey(ref manifest.KeyRef, opt Options) (Result, error) { return Result{}, err } res := Result{Removed: "key " + ref.String(), Revoked: del.Revoked, PrunedRecords: del.PrunedKeys} + if row.InOnePassword() { + res.VaultItems = append(res.VaultItems, fmt.Sprintf("item %q in vault %q", ref.KeyName, row.Vault)) + } // No config re-render and no known_hosts pruning: a deletable key is by // definition wired to no host, so neither the rendered config nor any pin // mentions it. diff --git a/src/internal/services/opstore/agentconfig.go b/src/internal/services/opstore/agentconfig.go new file mode 100644 index 0000000..44b3a29 --- /dev/null +++ b/src/internal/services/opstore/agentconfig.go @@ -0,0 +1,101 @@ +package opstore + +import ( + "bufio" + "fmt" + "os" + "path/filepath" + "runtime" + "strings" +) + +// The vaults the 1Password SSH agent serves when there is no agent.toml +// (checked against 1password.dev/ssh/agent/config, 2026-10-19). +var defaultAgentVaults = map[string]bool{"personal": true, "private": true, "employee": true} + +// AgentConfigPath is where the 1Password SSH agent reads agent.toml: +// $XDG_CONFIG_HOME/1Password/ssh/agent.toml when XDG_CONFIG_HOME is set, else +// ~/.config/1Password/ssh/agent.toml (on Windows, +// %LOCALAPPDATA%/1Password/config/ssh/agent.toml). +func AgentConfigPath() string { + if runtime.GOOS == "windows" { + return filepath.Join(os.Getenv("LOCALAPPDATA"), "1Password", "config", "ssh", "agent.toml") + } + if x := os.Getenv("XDG_CONFIG_HOME"); x != "" { + return filepath.Join(x, "1Password", "ssh", "agent.toml") + } + home, _ := os.UserHomeDir() + return filepath.Join(home, ".config", "1Password", "ssh", "agent.toml") +} + +// AgentServesVault reports whether the 1Password SSH agent offers every key in +// vault, and if not, what to change. Every key in the vault matters, not just +// today's: a rotation's .staged item has to be served to be verified. +// +// With no agent.toml the agent serves only the default vaults. Once the file +// exists it serves only what the file lists - even an empty file overrides the +// default - so the vault then needs its own whole-vault entry. +func AgentServesVault(vault, configPath string) (bool, string) { + entry := fmt.Sprintf("[[ssh-keys]]\nvault = %q", vault) + f, err := os.Open(configPath) + if err != nil { + if defaultAgentVaults[strings.ToLower(vault)] { + return true, "" + } + return false, fmt.Sprintf("the 1Password SSH agent serves only the Personal, Private and "+ + "Employee vaults by default, so keys in vault %q are never offered to ssh; add\n%s\nto %s", + vault, indent(entry), configPath) + } + defer func() { _ = f.Close() }() + itemsOnly := false + for _, e := range parseAgentConfig(f) { + if !strings.EqualFold(e["vault"], vault) { + continue + } + if e["item"] == "" { + return true, "" + } + itemsOnly = true + } + if itemsOnly { + return false, fmt.Sprintf("%s lists single items from vault %q but not the whole vault, so a "+ + "rotation's staged key is never offered and cannot be verified; add\n%s", configPath, vault, indent(entry)) + } + return false, fmt.Sprintf("%s exists, so the agent serves only what it lists, and vault %q is not "+ + "listed; add\n%s", configPath, vault, indent(entry)) +} + +func indent(s string) string { return " " + strings.ReplaceAll(s, "\n", "\n ") } + +// parseAgentConfig reads the [[ssh-keys]] entries of an agent.toml as key/value +// maps. It understands only what the file uses - array-of-tables headers, +// key = "string" pairs, and comments - which is all the agent accepts. +func parseAgentConfig(f *os.File) []map[string]string { + var entries []map[string]string + var cur map[string]string + sc := bufio.NewScanner(f) + for sc.Scan() { + line := strings.TrimSpace(sc.Text()) + if line == "" || strings.HasPrefix(line, "#") { + continue + } + if strings.HasPrefix(line, "[") { + cur = nil + if strings.TrimSpace(strings.Trim(line, "[]")) == "ssh-keys" { + cur = map[string]string{} + entries = append(entries, cur) + } + continue + } + key, val, ok := strings.Cut(line, "=") + if !ok || cur == nil { + continue + } + val = strings.TrimSpace(val) + if i := strings.Index(val, " #"); i >= 0 { + val = strings.TrimSpace(val[:i]) + } + cur[strings.TrimSpace(key)] = strings.Trim(val, `"'`) + } + return entries +} diff --git a/src/internal/services/opstore/agentconfig_test.go b/src/internal/services/opstore/agentconfig_test.go new file mode 100644 index 0000000..c11f912 --- /dev/null +++ b/src/internal/services/opstore/agentconfig_test.go @@ -0,0 +1,43 @@ +package opstore + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +func TestAgentServesVault(t *testing.T) { + dir := t.TempDir() + missing := filepath.Join(dir, "none.toml") + n := 0 + write := func(body string) string { + n++ + p := filepath.Join(dir, "agent"+string(rune('0'+n))+".toml") + if err := os.WriteFile(p, []byte(body), 0o600); err != nil { + t.Fatal(err) + } + return p + } + cases := []struct { + name, vault, cfg string + want bool + why string + }{ + {"default vault, no file", "Private", missing, true, ""}, + {"shared vault, no file", "Simtabi", missing, false, "by default"}, + {"empty file overrides the default", "Private", write(""), false, "not listed"}, + {"whole-vault entry", "Simtabi", write("# keys\n[[ssh-keys]]\nvault = \"simtabi\" # case-insensitive\n"), true, ""}, + {"item-only entry", "Simtabi", write("[[ssh-keys]]\nitem = \"bot\"\nvault = \"Simtabi\"\n"), false, "single items"}, + {"another vault listed", "Simtabi", write("[[ssh-keys]]\nvault = \"Private\"\n"), false, "not listed"}, + } + for _, c := range cases { + got, why := AgentServesVault(c.vault, c.cfg) + if got != c.want || (c.why != "" && !strings.Contains(why, c.why)) { + t.Errorf("%s: served=%v (%q), want %v containing %q", c.name, got, why, c.want, c.why) + } + if !got && !strings.Contains(why, `vault = "`+c.vault+`"`) { + t.Errorf("%s: the reason should show the entry to add: %q", c.name, why) + } + } +} diff --git a/src/internal/services/opstore/opstore.go b/src/internal/services/opstore/opstore.go new file mode 100644 index 0000000..618d0e1 --- /dev/null +++ b/src/internal/services/opstore/opstore.go @@ -0,0 +1,172 @@ +// Package opstore keeps SSH keys as items in 1Password, through the op CLI. +// +// The private key is generated inside 1Password and never written to disk: +// sshmgr only ever reads an item's public key back. Items are found by title +// (the key name) and then addressed by ID, so an edit or an archive cannot land +// on a different item that happens to share a title. +package opstore + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "fmt" + "os/exec" + "strings" + "time" +) + +// Item is the part of a 1Password SSH Key item sshmgr uses. +type Item struct { + ID string + Title string + Vault string + PublicKey string // OpenSSH one-line form + Fingerprint string // as 1Password reports it; may be empty +} + +// ErrNotFound means no item in the vault has the title. +var ErrNotFound = errors.New("not found in 1Password") + +// Client runs the op CLI. Bin is "op" unless a test points it elsewhere. +type Client struct { + Bin string + Timeout time.Duration +} + +// New returns a client for the op on PATH. +func New() *Client { return &Client{Bin: "op", Timeout: 60 * time.Second} } + +// Available reports why op cannot be used, or nil. +func (c *Client) Available() error { + if _, err := exec.LookPath(c.Bin); err != nil { + return fmt.Errorf("the 1Password CLI (op) is not on PATH: install it and sign in " + + "(https://developer.1password.com/docs/cli/get-started/)") + } + return nil +} + +// Create generates a new Ed25519 SSH key item titled title in vault and +// returns it. The private key stays in 1Password. +func (c *Client) Create(vault, title string) (Item, error) { + if err := c.Available(); err != nil { + return Item{}, err + } + // No --ssh-generate-key: Ed25519 is op's default, and the flag's documented + // form is only shown for RSA. + out, err := c.run("item", "create", "--category", "ssh", "--title", title, + "--vault", vault, "--format", "json") + if err != nil { + return Item{}, fmt.Errorf("op item create %q in vault %q: %w", title, vault, err) + } + it, err := parseItem(out) + if err != nil { + return Item{}, err + } + if it.PublicKey == "" { + return Item{}, fmt.Errorf("op created %q but returned no public key", title) + } + return it, nil +} + +// Get returns the item titled title in vault, or ErrNotFound. +func (c *Client) Get(vault, title string) (Item, error) { + if err := c.Available(); err != nil { + return Item{}, err + } + out, err := c.run("item", "get", title, "--vault", vault, "--format", "json") + if err != nil { + if isNotFound(err) { + return Item{}, fmt.Errorf("%q in vault %q: %w", title, vault, ErrNotFound) + } + return Item{}, fmt.Errorf("op item get %q in vault %q: %w", title, vault, err) + } + return parseItem(out) +} + +// Rename retitles the item with id. +func (c *Client) Rename(vault, id, title string) error { + if _, err := c.run("item", "edit", id, "--vault", vault, "--title", title); err != nil { + return fmt.Errorf("op item edit %s --title %q: %w", id, title, err) + } + return nil +} + +// Archive moves the item with id to 1Password's Archive, where it can still be +// restored. sshmgr never deletes an item outright. +func (c *Client) Archive(vault, id string) error { + if _, err := c.run("item", "delete", id, "--vault", vault, "--archive"); err != nil { + return fmt.Errorf("op item delete %s --archive: %w", id, err) + } + return nil +} + +func (c *Client) run(args ...string) ([]byte, error) { + timeout := c.Timeout + if timeout == 0 { + timeout = 60 * time.Second + } + ctx, cancel := context.WithTimeout(context.Background(), timeout) + defer cancel() + cmd := exec.CommandContext(ctx, c.Bin, args...) + var stdout, stderr bytes.Buffer + cmd.Stdout, cmd.Stderr = &stdout, &stderr + if err := cmd.Run(); err != nil { + msg := strings.TrimSpace(stderr.String()) + if msg == "" { + msg = err.Error() + } + return nil, errors.New(msg) + } + return stdout.Bytes(), nil +} + +// isNotFound reports op's own wording for a missing item ("\"x\" isn't an item +// in the \"V\" vault"), and nothing broader. A looser match also caught a +// missing vault ("isn't a vault"), a missing account and similar failures, which +// reconcile then answered by trying to create the item. +func isNotFound(err error) bool { + return strings.Contains(strings.ToLower(err.Error()), "isn't an item") +} + +// parseItem reads the fields sshmgr needs from `op item ... --format json`. +// The public key field is matched by id or by label, because 1Password +// documents neither, and both have been seen as "public_key" / "public key". +func parseItem(raw []byte) (Item, error) { + var doc struct { + ID string `json:"id"` + Title string `json:"title"` + Vault struct { + Name string `json:"name"` + } `json:"vault"` + Fields []struct { + ID string `json:"id"` + Label string `json:"label"` + Value string `json:"value"` + } `json:"fields"` + } + if err := json.Unmarshal(raw, &doc); err != nil { + return Item{}, fmt.Errorf("unexpected op output: %w", err) + } + it := Item{ID: doc.ID, Title: doc.Title, Vault: doc.Vault.Name} + for _, f := range doc.Fields { + switch norm(f.ID, f.Label) { + case "publickey": + it.PublicKey = strings.TrimSpace(f.Value) + case "fingerprint": + it.Fingerprint = strings.TrimSpace(f.Value) + } + } + return it, nil +} + +func norm(id, label string) string { + for _, s := range []string{id, label} { + k := strings.NewReplacer("_", "", " ", "", "-", "").Replace(strings.ToLower(s)) + if k == "publickey" || k == "fingerprint" { + return k + } + } + return "" +} diff --git a/src/internal/services/opstore/opstore_test.go b/src/internal/services/opstore/opstore_test.go new file mode 100644 index 0000000..86bc3cf --- /dev/null +++ b/src/internal/services/opstore/opstore_test.go @@ -0,0 +1,109 @@ +package opstore + +import ( + "errors" + "os" + "path/filepath" + "runtime" + "strings" + "testing" +) + +const itemJSON = `{"id":"abc123","title":"simtabi_github-ed25519","vault":{"id":"v1","name":"Simtabi"}, + "category":"SSH_KEY","fields":[ + {"id":"private_key","label":"private key","value":""}, + {"id":"public_key","label":"public key","value":"ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIExample"}, + {"id":"fingerprint","label":"fingerprint","value":"SHA256:abc"}, + {"id":"key_type","label":"key type","value":"ed25519"}]}` + +// fakeOp writes an op that logs its argv and runs body, and returns a client +// for it plus the log path. +func fakeOp(t *testing.T, body string) (*Client, string) { + t.Helper() + if runtime.GOOS == "windows" { + t.Skip("the fake op is a shell script") + } + dir := t.TempDir() + log := filepath.Join(dir, "argv") + script := "#!/bin/sh\nfor a in \"$@\"; do printf '[%s]' \"$a\" >> " + log + "; done\necho >> " + log + "\n" + body + bin := filepath.Join(dir, "op") + if err := os.WriteFile(bin, []byte(script), 0o755); err != nil { + t.Fatal(err) + } + return &Client{Bin: bin}, log +} + +func TestCreateReturnsThePublicKeyAndNeverAsksForThePrivateOne(t *testing.T) { + c, log := fakeOp(t, "cat <<'J'\n"+itemJSON+"\nJ\n") + it, err := c.Create("Simtabi", "simtabi_github-ed25519") + if err != nil { + t.Fatal(err) + } + if it.ID != "abc123" || it.PublicKey != "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIExample" || it.Fingerprint != "SHA256:abc" { + t.Errorf("item = %+v", it) + } + argv, _ := os.ReadFile(log) + want := "[item][create][--category][ssh][--title][simtabi_github-ed25519][--vault][Simtabi][--format][json]" + if !strings.Contains(string(argv), want) { + t.Errorf("argv = %s, want %s", argv, want) + } + if strings.Contains(string(argv), "private") || strings.Contains(string(argv), "reveal") { + t.Errorf("sshmgr must never read a private key out of 1Password: %s", argv) + } +} + +func TestGetReportsAMissingItemAsNotFound(t *testing.T) { + c, _ := fakeOp(t, `echo '[ERROR] 2026/10/19 "x" isn'"'"'t an item in the "Simtabi" vault.' >&2; exit 1`+"\n") + _, err := c.Get("Simtabi", "x") + if !errors.Is(err, ErrNotFound) { + t.Fatalf("err = %v, want ErrNotFound", err) + } +} + +func TestGetSurfacesOtherFailures(t *testing.T) { + c, _ := fakeOp(t, "echo '[ERROR] account is not signed in' >&2; exit 1\n") + _, err := c.Get("Simtabi", "x") + if err == nil || errors.Is(err, ErrNotFound) || !strings.Contains(err.Error(), "not signed in") { + t.Fatalf("err = %v, want the op error, not ErrNotFound", err) + } +} + +func TestPublicKeyFieldIsMatchedByLabelToo(t *testing.T) { + it, err := parseItem([]byte(`{"id":"i","title":"t","fields":[{"id":"x1","label":"Public Key","value":"ssh-ed25519 AAAA k"}]}`)) + if err != nil || it.PublicKey != "ssh-ed25519 AAAA k" { + t.Fatalf("item = %+v, err = %v", it, err) + } +} + +func TestEditAndArchiveAddressTheItemByID(t *testing.T) { + c, log := fakeOp(t, "exit 0\n") + if err := c.Rename("Simtabi", "abc123", "simtabi_github-ed25519.old"); err != nil { + t.Fatal(err) + } + if err := c.Archive("Simtabi", "abc123"); err != nil { + t.Fatal(err) + } + argv, _ := os.ReadFile(log) + for _, want := range []string{ + "[item][edit][abc123][--vault][Simtabi][--title][simtabi_github-ed25519.old]", + "[item][delete][abc123][--vault][Simtabi][--archive]", + } { + if !strings.Contains(string(argv), want) { + t.Errorf("argv = %s, want %s", argv, want) + } + } +} + +// A wrong vault is not a missing item: reconcile must not answer it by trying +// to create one. +func TestAMissingVaultIsNotAMissingItem(t *testing.T) { + for _, msg := range []string{ + `[ERROR] "Simtabbi" isn't a vault in this account.`, + `[ERROR] account not found`, + } { + c, _ := fakeOp(t, "echo '"+msg+"' >&2; exit 1\n") + if _, err := c.Get("Simtabbi", "x"); err == nil || errors.Is(err, ErrNotFound) { + t.Errorf("%q: err = %v, want a plain error, not ErrNotFound", msg, err) + } + } +} diff --git a/src/internal/services/pubkeys/pubkeys.go b/src/internal/services/pubkeys/pubkeys.go new file mode 100644 index 0000000..0043576 --- /dev/null +++ b/src/internal/services/pubkeys/pubkeys.go @@ -0,0 +1,186 @@ +// Package pubkeys regenerates public keys from the key they belong to: the +// private key file for a key on disk (ssh-keygen -y), or the vault item for a +// key kept in 1Password. It repairs a missing or wrong .pub; it never creates +// or changes a private key - that is reconcile's and rotate's job. +package pubkeys + +import ( + "errors" + "fmt" + "os" + "strings" + + "github.com/simtabi/ssh-manager/src/v3/internal/core/authkeys" + "github.com/simtabi/ssh-manager/src/v3/internal/core/inventory" + "github.com/simtabi/ssh-manager/src/v3/internal/core/manifest" + "github.com/simtabi/ssh-manager/src/v3/internal/services/keystore" + "github.com/simtabi/ssh-manager/src/v3/internal/services/keysvc" + "github.com/simtabi/ssh-manager/src/v3/internal/services/opstore" + "github.com/simtabi/ssh-manager/src/v3/internal/util/fs" + "github.com/simtabi/ssh-manager/src/v3/internal/util/perms" +) + +// What happened to one key's .pub. +const ( + Unchanged = "unchanged" // already matched its key + Created = "created" // was missing + Rewritten = "rewritten" // did not match its key + Skipped = "skipped" // nothing to derive it from, or needs a passphrase + Failed = "failed" +) + +// Result is the outcome for one key. +type Result struct { + Ref manifest.KeyRef + Action string + Detail string +} + +// Service regenerates public keys under one ~/.ssh. +type Service struct { + m *manifest.Manifest + inv *inventory.Inventory + ks *keystore.KeyStore + op *opstore.Client +} + +// New builds a Service. +func New(m *manifest.Manifest, inv *inventory.Inventory) *Service { + return &Service{m: m, inv: inv, ks: keystore.New(), op: opstore.New()} +} + +// Regenerate rebuilds the .pub of each row. With dryRun nothing is written and +// the inventory is left alone; the actions are what a real run would do. +func (s *Service) Regenerate(rows []keysvc.Row, dryRun bool) []Result { + out := make([]Result, 0, len(rows)) + for _, row := range rows { + out = append(out, s.one(row, dryRun)) + } + return out +} + +func (s *Service) one(row keysvc.Row, dryRun bool) Result { + res := Result{Ref: row.Ref} + source, skip, err := s.source(row) + switch { + case err != nil: + res.Action, res.Detail = Failed, err.Error() + return res + case skip != "": + res.Action, res.Detail = Skipped, skip + return res + } + + current := "" + if b, err := os.ReadFile(row.PublicPath); err == nil { + current = strings.TrimSpace(string(b)) + } + if current != "" && authkeys.KeyBody(current) == authkeys.KeyBody(source) { + res.Action = Unchanged + return res + } + res.Action = Created + if current != "" { + res.Action = Rewritten + res.Detail = "the .pub on disk was a different key; anything deployed from it " + + "needs `sshmgr deploy " + row.Ref.String() + "`" + } + if dryRun { + return res + } + if err := fs.WriteTextAtomic(row.PublicPath, withComment(source, current, row)+"\n", perms.PublicKeyMode); err != nil { + res.Action, res.Detail = Failed, err.Error() + return res + } + s.rekey(row) + return res +} + +// source is the public key the row's key really has, or why there is none. +func (s *Service) source(row keysvc.Row) (pub, skip string, err error) { + if row.InOnePassword() { + it, err := s.op.Get(row.Vault, row.Ref.KeyName) + switch { + case errors.Is(err, opstore.ErrNotFound): + return "", fmt.Sprintf("no item %q in 1Password vault %q (`sshmgr reconcile` creates it)", + row.Ref.KeyName, row.Vault), nil + case err != nil: + return "", "", err + case it.PublicKey == "": + return "", "", fmt.Errorf("1Password item %q has no public key", row.Ref.KeyName) + } + return it.PublicKey, "", nil + } + if !row.PrivateOnDisk { + return "", "the private key is missing, so there is nothing to derive from " + + "(`sshmgr reconcile` mints a new pair)", nil + } + derived, encrypted, err := s.ks.PublicFromPrivate(row.PrivatePath) + switch { + case err != nil: + return "", "", err + case encrypted: + return "", "the private key has a passphrase; derive it by hand: ssh-keygen -y -f " + + row.PrivatePath + " > " + row.PublicPath, nil + case derived == "": + return "", "", fmt.Errorf("%s is not a readable private key", row.PrivatePath) + } + return derived, "", nil +} + +// withComment keeps the comment the old .pub carried (it names the profile and +// host), falling back to the source's own, then to the key reference. +func withComment(source, current string, row keysvc.Row) string { + fields := strings.Fields(source) + if len(fields) < 2 { + return source + } + comment := "" + if old := strings.Fields(current); len(old) > 2 { + comment = strings.Join(old[2:], " ") + } else if len(fields) > 2 { + comment = strings.Join(fields[2:], " ") + } else { + comment = row.Ref.String() + } + return fields[0] + " " + fields[1] + " " + comment +} + +// rekey moves the key's inventory record to the fingerprint of the regenerated +// .pub. The record is keyed by fingerprint, and a wrong .pub had recorded the +// wrong one - which made expiry and deployments describe a key that is not the +// one in use. +func (s *Service) rekey(row keysvc.Row) { + fp, err := s.ks.Fingerprint(row.PublicPath) + if err != nil || fp == "" { + return + } + for old, rec := range s.inv.Keys { + if rec.Path == row.IdentityFile && old != fp { + delete(s.inv.Keys, old) + s.inv.Keys[fp] = rec + return + } + } +} + +// Format renders results, one line per key, then a tally. +func Format(results []Result, dryRun bool) string { + var b strings.Builder + counts := map[string]int{} + for _, r := range results { + counts[r.Action]++ + fmt.Fprintf(&b, "%-9s %s", r.Action, r.Ref) + if r.Detail != "" { + fmt.Fprintf(&b, " - %s", r.Detail) + } + b.WriteString("\n") + } + prefix := "" + if dryRun { + prefix = "(dry run) " + } + fmt.Fprintf(&b, "%s%d key(s): %d unchanged, %d created, %d rewritten, %d skipped, %d failed", + prefix, len(results), counts[Unchanged], counts[Created], counts[Rewritten], counts[Skipped], counts[Failed]) + return b.String() +} diff --git a/src/internal/services/pubkeys/pubkeys_test.go b/src/internal/services/pubkeys/pubkeys_test.go new file mode 100644 index 0000000..59e2f79 --- /dev/null +++ b/src/internal/services/pubkeys/pubkeys_test.go @@ -0,0 +1,149 @@ +package pubkeys + +import ( + "encoding/json" + "os" + "os/exec" + "path/filepath" + "runtime" + "strings" + "testing" + + "github.com/simtabi/ssh-manager/src/v3/internal/core/inventory" + "github.com/simtabi/ssh-manager/src/v3/internal/core/manifest" + "github.com/simtabi/ssh-manager/src/v3/internal/services/keystore" + "github.com/simtabi/ssh-manager/src/v3/internal/services/keysvc" +) + +const fileManifest = `{"version":1,"profiles":{"work":{"hosts":[ + {"alias":"a","hostname":"h","user":"u","key_name":"work_a-ed25519"}, + {"alias":"b","hostname":"h","user":"u","key_name":"work_b-ed25519"}]}}}` + +type fx struct { + m *manifest.Manifest + inv *inventory.Inventory + ssh string +} + +func setup(t *testing.T, raw string) fx { + t.Helper() + if _, err := exec.LookPath("ssh-keygen"); err != nil { + t.Skip("ssh-keygen not on PATH") + } + var m manifest.Manifest + if err := json.Unmarshal([]byte(raw), &m); err != nil { + t.Fatal(err) + } + return fx{&m, inventory.New(), t.TempDir()} +} + +func (f fx) mint(t *testing.T, ref manifest.KeyRef, passphrase string) string { + t.Helper() + priv := f.m.KeyPath(f.ssh, ref) + if err := os.MkdirAll(filepath.Dir(priv), 0o700); err != nil { + t.Fatal(err) + } + // ssh-keygen directly: keystore.Generate hands a passphrase over through the + // running binary as askpass helper, and in a test that binary is the test, + // which would run itself instead of answering. + if out, err := exec.Command("ssh-keygen", "-q", "-t", "ed25519", "-N", passphrase, "-C", ref.String(), "-f", priv).CombinedOutput(); err != nil { + t.Fatalf("ssh-keygen: %v %s", err, out) + } + fp, err := keystore.New().Fingerprint(priv + ".pub") + if err != nil { + t.Fatal(err) + } + f.inv.Record(fp, inventory.KeyRecord{Profile: ref.Profile, Path: f.m.IdentityFile(ref.Profile, ref.KeyName), Type: "ed25519"}) + return priv +} + +func (f fx) regen(t *testing.T, dry bool) map[string]Result { + t.Helper() + rows, err := keysvc.New(f.m, f.inv, f.ssh).Select(keysvc.Selection{All: true}) + if err != nil { + t.Fatal(err) + } + out := map[string]Result{} + for _, r := range New(f.m, f.inv).Regenerate(rows, dry) { + out[r.Ref.KeyName] = r + } + return out +} + +func TestRegenerateCreatesRewritesAndLeavesAlone(t *testing.T) { + f := setup(t, fileManifest) + a := f.mint(t, manifest.KeyRef{Profile: "work", KeyName: "work_a-ed25519"}, "") + b := f.mint(t, manifest.KeyRef{Profile: "work", KeyName: "work_b-ed25519"}, "") + aPub, _ := os.ReadFile(a + ".pub") + if err := os.Remove(a + ".pub"); err != nil { + t.Fatal(err) + } + // b's .pub now holds a's key: the wrong public key at the right path. + bPubBefore, _ := os.ReadFile(b + ".pub") + if err := os.WriteFile(b+".pub", aPub, 0o644); err != nil { + t.Fatal(err) + } + + dry := f.regen(t, true) + if dry["work_a-ed25519"].Action != Created || dry["work_b-ed25519"].Action != Rewritten { + t.Fatalf("dry run = %+v", dry) + } + if _, err := os.Stat(a + ".pub"); err == nil { + t.Fatal("a dry run wrote a file") + } + + got := f.regen(t, false) + if got["work_a-ed25519"].Action != Created || got["work_b-ed25519"].Action != Rewritten { + t.Fatalf("results = %+v", got) + } + if !strings.Contains(got["work_b-ed25519"].Detail, "sshmgr deploy") { + t.Errorf("a rewritten .pub should point at redeploying: %q", got["work_b-ed25519"].Detail) + } + bPubAfter, _ := os.ReadFile(b + ".pub") + if strings.Fields(string(bPubAfter))[1] != strings.Fields(string(bPubBefore))[1] { + t.Error("b's .pub was not restored to b's own key") + } + if again := f.regen(t, false); again["work_a-ed25519"].Action != Unchanged || again["work_b-ed25519"].Action != Unchanged { + t.Errorf("a second run should change nothing: %+v", again) + } +} + +func TestRegenerateSkipsWhatItCannotDerive(t *testing.T) { + f := setup(t, fileManifest) + f.mint(t, manifest.KeyRef{Profile: "work", KeyName: "work_a-ed25519"}, "s3cret-passphrase") + got := f.regen(t, false) + if got["work_a-ed25519"].Action != Skipped || !strings.Contains(got["work_a-ed25519"].Detail, "passphrase") { + t.Errorf("encrypted key: %+v", got["work_a-ed25519"]) + } + if got["work_b-ed25519"].Action != Skipped || !strings.Contains(got["work_b-ed25519"].Detail, "private key is missing") { + t.Errorf("no private key: %+v", got["work_b-ed25519"]) + } +} + +func TestRegenerateReadsAOnePasswordKeyFromItsItem(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("the fake op is a shell script") + } + f := setup(t, `{"version":1,"profiles":{"bot":{"key_store":"1password","op_vault":"V","hosts":[ + {"alias":"g","hostname":"github.com","user":"git","key_name":"bot_g-ed25519"}]}}}`) + dir := t.TempDir() + if out, err := exec.Command("ssh-keygen", "-q", "-t", "ed25519", "-N", "", "-f", filepath.Join(dir, "k")).CombinedOutput(); err != nil { + t.Fatalf("%v %s", err, out) + } + pub, _ := os.ReadFile(filepath.Join(dir, "k.pub")) + line := strings.Join(strings.Fields(string(pub))[:2], " ") + op := "#!/bin/sh\necho '{\"id\":\"i1\",\"title\":\"bot_g-ed25519\",\"fields\":[{\"id\":\"public_key\",\"value\":\"" + line + "\"}]}'\n" + if err := os.WriteFile(filepath.Join(dir, "op"), []byte(op), 0o755); err != nil { + t.Fatal(err) + } + t.Setenv("PATH", dir+string(os.PathListSeparator)+os.Getenv("PATH")) + + got := f.regen(t, false) + if got["bot_g-ed25519"].Action != Created { + t.Fatalf("result = %+v", got["bot_g-ed25519"]) + } + written, _ := os.ReadFile(f.m.KeyPath(f.ssh, manifest.KeyRef{Profile: "bot", KeyName: "bot_g-ed25519"}) + ".pub") + if !strings.HasPrefix(string(written), line) { + t.Errorf(".pub = %q, want the item's public key", written) + } +} diff --git a/src/internal/services/reconciler/reconciler.go b/src/internal/services/reconciler/reconciler.go index 679ea43..b214a5a 100644 --- a/src/internal/services/reconciler/reconciler.go +++ b/src/internal/services/reconciler/reconciler.go @@ -7,6 +7,7 @@ package reconciler import ( + "errors" "fmt" "path/filepath" "sort" @@ -16,6 +17,7 @@ import ( "github.com/simtabi/ssh-manager/src/v3/internal/core/manifest" "github.com/simtabi/ssh-manager/src/v3/internal/services/configsvc" "github.com/simtabi/ssh-manager/src/v3/internal/services/keystore" + "github.com/simtabi/ssh-manager/src/v3/internal/services/opstore" "github.com/simtabi/ssh-manager/src/v3/internal/util/fs" "github.com/simtabi/ssh-manager/src/v3/internal/util/homeperms" "github.com/simtabi/ssh-manager/src/v3/internal/util/log" @@ -99,6 +101,7 @@ type Reconciler struct { m *manifest.Manifest inv *inventory.Inventory ks *keystore.KeyStore + op *opstore.Client // for profiles whose key_store is 1password cfg *configsvc.Service } @@ -106,7 +109,7 @@ type Reconciler struct { // elsewhere. func New(p paths.Paths, m *manifest.Manifest, inv *inventory.Inventory, emitUseKeychain bool) *Reconciler { return &Reconciler{ - p: p, m: m, inv: inv, ks: keystore.New(), + p: p, m: m, inv: inv, ks: keystore.New(), op: opstore.New(), cfg: configsvc.New(p.SSHDir, m, emitUseKeychain), } } @@ -115,6 +118,16 @@ func (r *Reconciler) privPath(profile, keyName string) string { return r.m.KeyPath(r.p.SSHDir, manifest.KeyRef{Profile: profile, KeyName: keyName}) } +// present reports whether a key's on-disk part exists: the private key for a +// key on disk, the public key for one kept in 1Password. +func (r *Reconciler) present(ref manifest.KeyRef) bool { + priv := r.privPath(ref.Profile, ref.KeyName) + if r.m.Profiles[ref.Profile].InOnePassword() { + return fs.Exists(priv + ".pub") + } + return fs.Exists(priv) +} + // Reconcile rebuilds the tree (mints missing keys, renders config, fixes perms, // validates). With dryRun it only plans key work and previews the config write. func (r *Reconciler) Reconcile(dryRun bool, passphrase string) (*ReconcileResult, error) { @@ -217,7 +230,7 @@ func (r *Reconciler) Mint(selector, passphrase string, overwrite map[manifest.Ke // key is already on disk: a command that adds a key must never regenerate an // identity that exists, since the replaced key is what remote targets trust. func (r *Reconciler) MintRef(ref manifest.KeyRef, passphrase string) (*MintedKey, error) { - if fs.Exists(r.privPath(ref.Profile, ref.KeyName)) { + if r.present(ref) { return nil, nil } hosts, err := r.m.HostsForKey(ref) @@ -280,7 +293,7 @@ func (r *Reconciler) planMint(selector string) (toMint, existing []plannedKey, e } pk := plannedKey{Ref: ref, Hosts: hosts} priv := r.privPath(ref.Profile, ref.KeyName) - if fs.Exists(priv) { + if r.present(ref) { existing = append(existing, pk) continue } @@ -300,14 +313,52 @@ func (r *Reconciler) planMint(selector string) (toMint, existing []plannedKey, e // for one identity, the old one still deployed and the new one not. "" when // the key is nowhere else. func (r *Reconciler) misplaced(ref manifest.KeyRef, priv string) string { - for _, p := range manifest.GlobProfileTree(r.p.SSHDir, ref.KeyName) { - if p != priv && filepath.Base(filepath.Dir(p)) == ref.Profile { + name, want := ref.KeyName, priv + if r.m.Profiles[ref.Profile].InOnePassword() { + name, want = name+".pub", priv+".pub" // only the public half is ever on disk + } + for _, p := range manifest.GlobProfileTree(r.p.SSHDir, name) { + if p != want && filepath.Base(filepath.Dir(p)) == ref.Profile { return p } } return "" } +// mintInOnePassword creates the key as an SSH Key item titled with the key name +// in the profile's vault, and writes only its public half to pub. An item that +// already has that title is reused rather than duplicated: a .pub deleted from +// disk is restored from the vault, never replaced by a second key. +func (r *Reconciler) mintInOnePassword(ref manifest.KeyRef, pub string, overwrite bool) (keystore.GenResult, error) { + if overwrite { + return keystore.GenResult{}, fmt.Errorf("key %s is kept in 1Password; replace it with "+ + "`sshmgr rotate %s`, which stages, deploys and verifies the new key first", ref, ref) + } + vault := r.m.Profiles[ref.Profile].OPVault + it, err := r.op.Get(vault, ref.KeyName) + switch { + case err == nil: + case errors.Is(err, opstore.ErrNotFound): + if it, err = r.op.Create(vault, ref.KeyName); err != nil { + return keystore.GenResult{}, err + } + default: + return keystore.GenResult{}, err + } + if it.PublicKey == "" { + return keystore.GenResult{}, fmt.Errorf("1Password item %q in vault %q has no public key - "+ + "is it an SSH Key item?", ref.KeyName, vault) + } + if err := fs.WriteTextAtomic(pub, it.PublicKey+"\n", perms.PublicKeyMode); err != nil { + return keystore.GenResult{}, err + } + fp, err := r.ks.Fingerprint(pub) + if err != nil { + return keystore.GenResult{}, err + } + return keystore.GenResult{Fingerprint: fp}, nil +} + // planMatches reports whether a key is in scope for selector: everything when // empty, else the profile that owns it, the key itself (by name or in the // "profile/key" form), or any host alias that uses it. @@ -378,7 +429,14 @@ func (r *Reconciler) mintOne(pk plannedKey, passphrase string, overwrite bool) ( } keyType := r.m.KeyTypeFor(ref) comment := fmt.Sprintf("%s/%s %s", ref.Profile, subject, inventory.Today()) - gen, err := r.ks.Generate(priv, keyType, comment, passphrase, overwrite) + var gen keystore.GenResult + var err error + if r.m.Profiles[ref.Profile].InOnePassword() { + gen, err = r.mintInOnePassword(ref, priv+".pub", overwrite) + keyType = "ed25519" // what 1Password generates + } else { + gen, err = r.ks.Generate(priv, keyType, comment, passphrase, overwrite) + } if err != nil { return MintedKey{}, err } diff --git a/src/internal/services/rotator/onepassword.go b/src/internal/services/rotator/onepassword.go new file mode 100644 index 0000000..672b99f --- /dev/null +++ b/src/internal/services/rotator/onepassword.go @@ -0,0 +1,277 @@ +package rotator + +import ( + "errors" + "fmt" + "os" + "path/filepath" + "strings" + + "github.com/simtabi/ssh-manager/src/v3/internal/core/manifest" + "github.com/simtabi/ssh-manager/src/v3/internal/services/opstore" + "github.com/simtabi/ssh-manager/src/v3/internal/util/fs" + "github.com/simtabi/ssh-manager/src/v3/internal/util/log" + "github.com/simtabi/ssh-manager/src/v3/internal/util/perms" +) + +// Item titles in the vault. The live key is titled with the key name, exactly +// one predecessor sits at .old (as old/ holds one on disk), and a rotation in +// flight is .staged until it commits. +func stagedTitle(keyName string) string { return keyName + ".staged" } +func oldTitle(keyName string) string { return keyName + ".old" } + +// agentFor is the agent a verify login uses: the 1Password agent when the +// profile keeps its keys there and the identity is a public key; none for a +// key file. +func (r *Rotator) agentFor(profile, identPath string) string { + if identPath == "" || !r.m.Profiles[profile].InOnePassword() || !strings.HasSuffix(identPath, ".pub") { + return "" + } + return r.m.Defaults.OnePasswordAgentSocket() +} + +// rotateOnePassword is Rotate for a profile whose keys live in 1Password. The +// new key is generated inside 1Password as .staged, deployed and verified +// through the 1Password agent, and only then retitled into place, the +// outgoing item becoming .old. Nothing private is ever written to disk. +// +// It also moves a profile *into* 1Password: when the outgoing key is still a +// private key file (key_store was just switched), that pair is archived to +// old/ the way a file rotation archives it, and the new key is the vault item. +func (r *Rotator) rotateOnePassword(profile, keyName string, hosts []manifest.Host, allowUnverified bool) (RotateReport, error) { + if err := r.op.Available(); err != nil { + return RotateReport{}, err + } + vault := r.m.Profiles[profile].OPVault + pdir := r.dir(profile) + curPriv := filepath.Join(pdir, keyName) + curPub := curPriv + ".pub" + if !exists(curPub) { + return RotateReport{}, fmt.Errorf("key not present: %s - run `sshmgr reconcile` first", curPub) + } + report := RotateReport{KeyName: keyName} + if fp, err := r.ks.Fingerprint(curPub); err == nil { + report.OldFingerprint = fp + } + oldPubText := readFile(curPub) + + if msgs := r.unreachable(hosts); len(msgs) > 0 { + report.Message = "cannot rotate - " + strings.Join(msgs, "; ") + log.Audit(r.p.AuditLog(), "rotate.unreachable", log.Field{Key: "key", Value: keyName}) + return report, nil + } + + // 1. Stage: a fresh item in the vault, its public half in .staging/. + if leftover, err := r.op.Get(vault, stagedTitle(keyName)); err == nil { + if err := r.op.Archive(vault, leftover.ID); err != nil { + return RotateReport{}, fmt.Errorf("a staged key from an earlier rotation is still in the vault "+ + "and could not be archived: %w", err) + } + } else if !errors.Is(err, opstore.ErrNotFound) { + return RotateReport{}, err + } + staged, err := r.op.Create(vault, stagedTitle(keyName)) + if err != nil { + return RotateReport{}, err + } + staging := filepath.Join(pdir, ".staging") + _ = os.RemoveAll(staging) + discard := func() { + _ = os.RemoveAll(staging) + _ = r.op.Archive(vault, staged.ID) + } + if err := fs.EnsureDir(staging, perms.DirMode); err != nil { + discard() + return RotateReport{}, err + } + stagedPub := filepath.Join(staging, keyName+".pub") + if err := fs.WriteTextAtomic(stagedPub, staged.PublicKey+"\n", perms.PublicKeyMode); err != nil { + discard() + return RotateReport{}, err + } + if fp, err := r.ks.Fingerprint(stagedPub); err == nil { + report.NewFingerprint = fp + } + + // 2+3. Deploy and verify through the 1Password agent. + stagedPubText := readFile(stagedPub) + var results []TargetResult + for _, h := range hosts { + prov := r.provider(h) + tr := TargetResult{Alias: h.Alias, Provider: prov.Name()} + tgt := r.target(h, profile, stagedPub, stagedPubText, stagedPub) + out := prov.Deploy(tgt) + tr.Deployed = out.Method == "manual" || out.Verified + tr.Verified = prov.Verify(tgt) + results = append(results, tr) + } + report.Targets = results + ready := allTrue(results, func(t TargetResult) bool { return t.Verified }) || + (allowUnverified && allTrue(results, func(t TargetResult) bool { return t.Deployed })) + if !ready { + for i, h := range hosts { + if results[i].Deployed { + _ = r.provider(h).Remove(r.target(h, profile, stagedPub, stagedPubText, "")) + } + } + discard() + report.Message = "verification failed on one or more targets - the staged key was archived " + + "in 1Password (and pulled back from any target it reached), active key untouched. " + + "(Use --allow-unverified to accept manual/web-panel targets.)" + log.Audit(r.p.AuditLog(), "rotate.abort", log.Field{Key: "key", Value: keyName}, log.Field{Key: "old", Value: report.OldFingerprint}) + return report, nil + } + + // 4. Commit. The outgoing key becomes the predecessor, in the vault or on + // disk, then the staged item takes the live title. Each step that fails + // puts back what had already moved. + oldDir := filepath.Join(pdir, "old") + if err := fs.EnsureDir(oldDir, perms.DirMode); err != nil { + discard() + return RotateReport{}, fmt.Errorf("could not prepare the archive directory, so the rotation "+ + "was not committed (the active key is untouched): %w", err) + } + // note tells a failure that happens after the earlier predecessor was + // archived where that item went: everything else is put back, but an + // archive is not undone. + note := func(err error) error { return err } + if prev, err := r.op.Get(vault, oldTitle(keyName)); err == nil { + // One predecessor, as on disk: the older one goes to the vault's + // Archive, where it can still be restored. + if err := r.op.Archive(vault, prev.ID); err != nil { + discard() + return RotateReport{}, fmt.Errorf("could not archive the earlier predecessor %q, so the "+ + "rotation was not committed: %w", oldTitle(keyName), err) + } + note = func(err error) error { + return fmt.Errorf("%w\n(the earlier predecessor %q had already been moved to the Archive "+ + "of vault %q; restore it from there if you still need it)", err, oldTitle(keyName), vault) + } + } + var undo []func() + rollbackCommit := func() { + for i := len(undo) - 1; i >= 0; i-- { + undo[i]() + } + } + cur, err := r.op.Get(vault, keyName) + switch { + case err == nil: + if err := r.op.Rename(vault, cur.ID, oldTitle(keyName)); err != nil { + discard() + return RotateReport{}, note(fmt.Errorf("could not retitle the outgoing key, so the rotation "+ + "was not committed (the active key is untouched): %w", err)) + } + undo = append(undo, func() { _ = r.op.Rename(vault, cur.ID, keyName) }) + case errors.Is(err, opstore.ErrNotFound) && exists(curPriv): + // Moving into 1Password: the outgoing key is a file pair. + oldPriv := filepath.Join(oldDir, keyName) + if err := os.Rename(curPriv, oldPriv); err != nil { + discard() + return RotateReport{}, note(fmt.Errorf("could not archive the outgoing key file, so the "+ + "rotation was not committed: %w", err)) + } + undo = append(undo, func() { _ = os.Rename(oldPriv, curPriv) }) + case errors.Is(err, opstore.ErrNotFound): + // A .pub with no item and no private key: nothing to retire in the vault. + default: + discard() + return RotateReport{}, note(err) + } + if err := r.op.Rename(vault, staged.ID, keyName); err != nil { + rollbackCommit() + discard() + return RotateReport{}, note(fmt.Errorf("could not give the new key its title, so the rotation was "+ + "undone and the previous key is active again: %w", err)) + } + oldPub := filepath.Join(oldDir, keyName+".pub") + if err := copyFile(curPub, oldPub); err != nil { + _ = r.op.Rename(vault, staged.ID, stagedTitle(keyName)) + rollbackCommit() + discard() + return RotateReport{}, note(fmt.Errorf("could not archive the outgoing public key, so the rotation "+ + "was undone: %w", err)) + } + if err := os.Rename(stagedPub, curPub); err != nil { + _ = r.op.Rename(vault, staged.ID, stagedTitle(keyName)) + rollbackCommit() + discard() + return RotateReport{}, note(fmt.Errorf("could not move the new public key into place, so the "+ + "rotation was undone: %w", err)) + } + _ = perms.SetPerms(curPub, perms.PublicKeyMode) + _ = perms.SetPerms(oldPub, perms.PublicKeyMode) + _ = os.RemoveAll(staging) + + for i := range hosts { + results[i].Revoked = r.provider(hosts[i]).Remove(r.target(hosts[i], profile, oldPub, oldPubText, "")) + } + report.Targets = results + r.updateInventory(profile, keyName, report, results) + report.Committed = true + log.Audit(r.p.AuditLog(), "rotate", log.Field{Key: "key", Value: keyName}, log.Field{Key: "store", Value: "1password"}, + log.Field{Key: "old", Value: report.OldFingerprint}, log.Field{Key: "new", Value: report.NewFingerprint}) + return report, nil +} + +// rollbackOnePassword is Rollback for a profile whose keys live in 1Password: +// the .old item takes the live title back, the rotated-in item goes to the +// vault's Archive (recoverable, never deleted), and the predecessor's public key +// returns from old/. +func (r *Rotator) rollbackOnePassword(profile, keyName string, hosts []manifest.Host) (RotateReport, error) { + if err := r.op.Available(); err != nil { + return RotateReport{}, err + } + vault := r.m.Profiles[profile].OPVault + pdir := r.dir(profile) + curPub := filepath.Join(pdir, keyName+".pub") + oldPub := filepath.Join(pdir, "old", keyName+".pub") + if exists(filepath.Join(pdir, "old", keyName)) { + return RotateReport{}, fmt.Errorf("the predecessor of %q is a key file from before the profile "+ + "moved to 1Password; set key_store back to %q to roll back to it", keyName, manifest.KeyStoreFile) + } + prev, err := r.op.Get(vault, oldTitle(keyName)) + if errors.Is(err, opstore.ErrNotFound) || !exists(oldPub) { + return RotateReport{}, fmt.Errorf("no predecessor to roll back to for %q (want item %q in vault %q "+ + "and %s)", keyName, oldTitle(keyName), vault, oldPub) + } + if err != nil { + return RotateReport{}, err + } + report := RotateReport{KeyName: keyName} + if fp, err := r.ks.Fingerprint(curPub); err == nil { + report.OldFingerprint = fp + } + curPubText := readFile(curPub) + if fp, err := r.ks.Fingerprint(oldPub); err == nil { + report.NewFingerprint = fp + } + + cur, err := r.op.Get(vault, keyName) + if err != nil && !errors.Is(err, opstore.ErrNotFound) { + return RotateReport{}, err + } + if err == nil { + // Out of the live title first, so the predecessor can take it. + if err := r.op.Rename(vault, cur.ID, keyName+".rolled-back"); err != nil { + return RotateReport{}, fmt.Errorf("could not retitle the rotated-in key, so nothing was "+ + "rolled back: %w", err) + } + } + if err := r.op.Rename(vault, prev.ID, keyName); err != nil { + if cur.ID != "" { + _ = r.op.Rename(vault, cur.ID, keyName) + } + return RotateReport{}, fmt.Errorf("could not restore the predecessor's title, so nothing was "+ + "rolled back: %w", err) + } + if err := os.Rename(oldPub, curPub); err != nil { + return RotateReport{}, fmt.Errorf("the predecessor is the live item %q again, but its public key "+ + "could not be moved back from %s to %s - move it by hand: %w", keyName, oldPub, curPub, err) + } + _ = perms.SetPerms(curPub, perms.PublicKeyMode) + if cur.ID != "" { + _ = r.op.Archive(vault, cur.ID) + } + return r.finishRollback(profile, keyName, hosts, curPub, curPub, curPubText, report) +} diff --git a/src/internal/services/rotator/onepassword_test.go b/src/internal/services/rotator/onepassword_test.go new file mode 100644 index 0000000..74b1623 --- /dev/null +++ b/src/internal/services/rotator/onepassword_test.go @@ -0,0 +1,249 @@ +package rotator + +import ( + "encoding/json" + "os" + "os/exec" + "path/filepath" + "runtime" + "sort" + "strings" + "testing" + + "github.com/simtabi/ssh-manager/src/v3/internal/core/inventory" + "github.com/simtabi/ssh-manager/src/v3/internal/core/manifest" + "github.com/simtabi/ssh-manager/src/v3/internal/services/reconciler" + "github.com/simtabi/ssh-manager/src/v3/internal/util/paths" +) + +// fakeOpScript is a stateful stand-in for the 1Password CLI: one directory per +// item (title, id, a real Ed25519 public key generated by ssh-keygen), and an +// archive directory for archived items. It answers only the calls sshmgr makes. +const fakeOpScript = `#!/bin/sh +V="$FAKE_OP_VAULT"; mkdir -p "$V/items" "$V/archive" +cmd="$1 $2"; shift 2 +json() { d="$1"; printf '{"id":"%s","title":"%s","vault":{"name":"V"},"fields":[{"id":"public_key","label":"public key","value":"%s"}]}\n' "$(cat $d/id)" "$(cat $d/title)" "$(cat $d/pub)"; } +find_title() { for d in "$V"/items/*; do [ -d "$d" ] && [ "$(cat $d/title)" = "$1" ] && echo "$d"; done; } +case "$cmd" in +"item create") + title=""; while [ $# -gt 0 ]; do [ "$1" = "--title" ] && title="$2"; shift; done + id="i$(ls "$V/items" "$V/archive" | wc -l | tr -d ' ')$$"; d="$V/items/$id"; mkdir -p "$d" + ssh-keygen -q -t ed25519 -N '' -C "$title" -f "$d/k" >/dev/null + echo "$id" > "$d/id"; echo "$title" > "$d/title"; cut -d' ' -f1,2 "$d/k.pub" > "$d/pub"; rm -f "$d/k" + json "$d" ;; +"item get") + d=$(find_title "$1"); [ -z "$d" ] && { echo "[ERROR] \"$1\" isn't an item in the vault." >&2; exit 1; } + json "$d" ;; +"item edit") + id="$1"; shift; while [ $# -gt 0 ]; do + if [ "$1" = "--title" ]; then + [ -n "$FAKE_OP_FAIL_FROM" ] && [ "$(cat $V/items/$id/title)" = "$FAKE_OP_FAIL_FROM" ] && { echo "[ERROR] edit refused" >&2; exit 1; } + echo "$2" > "$V/items/$id/title" + fi; shift; done ;; +"item delete") + mv "$V/items/$1" "$V/archive/$1" ;; +*) echo "fake op: unsupported $cmd" >&2; exit 2 ;; +esac +` + +// opVault installs the fake op on PATH and returns a function listing the live +// item titles, sorted. +func opVault(t *testing.T) (titles func() []string, archived func() int) { + t.Helper() + if runtime.GOOS == "windows" { + t.Skip("the fake op is a shell script") + } + if _, err := exec.LookPath("ssh-keygen"); err != nil { + t.Skip("ssh-keygen not on PATH") + } + bin := t.TempDir() + vault := t.TempDir() + if err := os.WriteFile(filepath.Join(bin, "op"), []byte(fakeOpScript), 0o755); err != nil { + t.Fatal(err) + } + t.Setenv("PATH", bin+string(os.PathListSeparator)+os.Getenv("PATH")) + t.Setenv("FAKE_OP_VAULT", vault) + titles = func() []string { + var out []string + entries, _ := os.ReadDir(filepath.Join(vault, "items")) + for _, e := range entries { + b, _ := os.ReadFile(filepath.Join(vault, "items", e.Name(), "title")) + out = append(out, strings.TrimSpace(string(b))) + } + sort.Strings(out) + return out + } + archived = func() int { + entries, _ := os.ReadDir(filepath.Join(vault, "archive")) + return len(entries) + } + return titles, archived +} + +const opManifestJSON = `{"version":1,"defaults":{"key_type":"ed25519","rotate_after_days":365}, + "profiles":{"bot":{"category":"orgs","key_store":"1password","op_vault":"Simtabi","hosts":[ + {"alias":"panel","hostname":"ploi.example","user":"x","provider":"ploi","key_name":"bot_panel-ed25519"}]}}}` + +func opSetup(t *testing.T, raw string) (paths.Paths, *manifest.Manifest, *inventory.Inventory) { + t.Helper() + var m manifest.Manifest + if err := json.Unmarshal([]byte(raw), &m); err != nil { + t.Fatal(err) + } + base := t.TempDir() + return paths.Paths{SSHDir: filepath.Join(base, ".ssh"), ConfigDir: filepath.Join(base, "cfg")}, &m, inventory.New() +} + +func TestOnePasswordKeyIsMintedInTheVaultWithOnlyItsPublicHalfOnDisk(t *testing.T) { + titles, _ := opVault(t) + p, m, inv := opSetup(t, opManifestJSON) + if _, err := reconciler.New(p, m, inv, false).Reconcile(false, ""); err != nil { + t.Fatal(err) + } + dir := filepath.Join(p.SSHDir, "profiles", "orgs", "bot") + if _, err := os.Stat(filepath.Join(dir, "bot_panel-ed25519.pub")); err != nil { + t.Fatalf("public key not written: %v", err) + } + if _, err := os.Stat(filepath.Join(dir, "bot_panel-ed25519")); err == nil { + t.Fatal("a private key was written to disk for a 1Password profile") + } + if got := strings.Join(titles(), ","); got != "bot_panel-ed25519" { + t.Errorf("vault items = %q", got) + } + cfg, _ := os.ReadFile(filepath.Join(p.SSHDir, "profiles", "orgs", "config")) + if !strings.Contains(string(cfg), "IdentityFile ~/.ssh/profiles/orgs/bot/bot_panel-ed25519.pub\n") || + !strings.Contains(string(cfg), "IdentityAgent \"") { + t.Errorf("rendered host should use the .pub and the 1Password agent:\n%s", cfg) + } + // A second reconcile finds the key present: no second item. + if _, err := reconciler.New(p, m, inv, false).Reconcile(false, ""); err != nil { + t.Fatal(err) + } + if n := len(titles()); n != 1 { + t.Errorf("reconcile created a duplicate item: %v", titles()) + } +} + +func TestOnePasswordRotateThenRollback(t *testing.T) { + titles, archived := opVault(t) + p, m, inv := opSetup(t, opManifestJSON) + if _, err := reconciler.New(p, m, inv, false).Reconcile(false, ""); err != nil { + t.Fatal(err) + } + pub := filepath.Join(p.SSHDir, "profiles", "orgs", "bot", "bot_panel-ed25519.pub") + before, _ := os.ReadFile(pub) + + rep, err := New(p, m, inv).Rotate("bot_panel-ed25519", true, "") + if err != nil || !rep.Committed { + t.Fatalf("rotate: committed=%v err=%v %+v", rep.Committed, err, rep) + } + after, _ := os.ReadFile(pub) + if string(after) == string(before) || rep.OldFingerprint == rep.NewFingerprint { + t.Fatal("the live public key did not change") + } + if got := strings.Join(titles(), ","); got != "bot_panel-ed25519,bot_panel-ed25519.old" { + t.Errorf("after rotate, vault = %q", got) + } + if old, _ := os.ReadFile(filepath.Join(filepath.Dir(pub), "old", "bot_panel-ed25519.pub")); string(old) != string(before) { + t.Error("the outgoing public key was not archived to old/") + } + + // A second rotation keeps exactly one predecessor; the older one is archived. + if rep, err := New(p, m, inv).Rotate("bot_panel-ed25519", true, ""); err != nil || !rep.Committed { + t.Fatalf("second rotate: %v %+v", err, rep) + } + if got := strings.Join(titles(), ","); got != "bot_panel-ed25519,bot_panel-ed25519.old" || archived() != 1 { + t.Errorf("after two rotations, vault = %q, archived = %d", got, archived()) + } + + mid, _ := os.ReadFile(pub) + rb, err := New(p, m, inv).Rollback("bot_panel-ed25519") + if err != nil || !rb.Committed { + t.Fatalf("rollback: %v %+v", err, rb) + } + restored, _ := os.ReadFile(pub) + if string(restored) == string(mid) { + t.Error("rollback left the rotated-in public key in place") + } + if got := strings.Join(titles(), ","); got != "bot_panel-ed25519" || archived() != 2 { + t.Errorf("after rollback, vault = %q (want only the restored key), archived = %d", got, archived()) + } + for _, e := range []string{".staging"} { + if _, err := os.Stat(filepath.Join(filepath.Dir(pub), e)); err == nil { + t.Errorf("%s left behind", e) + } + } +} + +// Switching key_store to 1password and rotating moves the profile into the +// vault: the old key file pair goes to old/, the new key exists only as an item. +func TestRotateMovesAFileKeyIntoOnePassword(t *testing.T) { + titles, _ := opVault(t) + fileJSON := strings.Replace(opManifestJSON, `"key_store":"1password","op_vault":"Simtabi",`, "", 1) + p, m, inv := opSetup(t, fileJSON) + if _, err := reconciler.New(p, m, inv, false).Reconcile(false, ""); err != nil { + t.Fatal(err) + } + dir := filepath.Join(p.SSHDir, "profiles", "orgs", "bot") + if _, err := os.Stat(filepath.Join(dir, "bot_panel-ed25519")); err != nil { + t.Fatalf("file key not minted: %v", err) + } + // Switch the store the way an edited manifest would: reload it with the + // 1Password fields that opManifestJSON carries. + var switched manifest.Manifest + if err := json.Unmarshal([]byte(opManifestJSON), &switched); err != nil { + t.Fatal(err) + } + m.Profiles["bot"] = switched.Profiles["bot"] + + rep, err := New(p, m, inv).Rotate("bot_panel-ed25519", true, "") + if err != nil || !rep.Committed { + t.Fatalf("rotate: %v %+v", err, rep) + } + if _, err := os.Stat(filepath.Join(dir, "bot_panel-ed25519")); err == nil { + t.Error("the private key file is still at the live path") + } + if _, err := os.Stat(filepath.Join(dir, "old", "bot_panel-ed25519")); err != nil { + t.Errorf("the outgoing key file was not archived to old/: %v", err) + } + if got := strings.Join(titles(), ","); got != "bot_panel-ed25519" { + t.Errorf("vault = %q, want the new key under the live title", got) + } + // Rolling back across the move needs the file store again, and says so. + if _, err := New(p, m, inv).Rollback("bot_panel-ed25519"); err == nil || + !strings.Contains(err.Error(), "key_store") { + t.Errorf("rollback across the move: err = %v, want a pointer at key_store", err) + } +} + +// A rotation that fails after archiving the earlier predecessor puts the live +// key back and says where the archived item went. +func TestFailedOnePasswordRotationNamesTheArchivedPredecessor(t *testing.T) { + titles, archived := opVault(t) + p, m, inv := opSetup(t, opManifestJSON) + if _, err := reconciler.New(p, m, inv, false).Reconcile(false, ""); err != nil { + t.Fatal(err) + } + if rep, err := New(p, m, inv).Rotate("bot_panel-ed25519", true, ""); err != nil || !rep.Committed { + t.Fatalf("first rotate: %v", err) + } + pub := filepath.Join(p.SSHDir, "profiles", "orgs", "bot", "bot_panel-ed25519.pub") + live, _ := os.ReadFile(pub) + + // The staged item cannot be retitled: the last vault step of the commit. + t.Setenv("FAKE_OP_FAIL_FROM", "bot_panel-ed25519.staged") + _, err := New(p, m, inv).Rotate("bot_panel-ed25519", true, "") + if err == nil || !strings.Contains(err.Error(), "moved to the Archive") { + t.Fatalf("err = %v, want it to name the archived predecessor", err) + } + t.Setenv("FAKE_OP_FAIL_FROM", "") + if got, _ := os.ReadFile(pub); string(got) != string(live) { + t.Error("the live public key changed although the rotation failed") + } + if got := strings.Join(titles(), ","); got != "bot_panel-ed25519" { + t.Errorf("vault = %q, want the live key back under its title", got) + } + if archived() < 2 { + t.Errorf("archived = %d, want the earlier predecessor and the staged key archived", archived()) + } +} diff --git a/src/internal/services/rotator/rotator.go b/src/internal/services/rotator/rotator.go index 5f581c1..7b6610c 100644 --- a/src/internal/services/rotator/rotator.go +++ b/src/internal/services/rotator/rotator.go @@ -17,6 +17,7 @@ import ( "github.com/simtabi/ssh-manager/src/v3/internal/core/manifest" "github.com/simtabi/ssh-manager/src/v3/internal/core/providers" "github.com/simtabi/ssh-manager/src/v3/internal/services/keystore" + "github.com/simtabi/ssh-manager/src/v3/internal/services/opstore" "github.com/simtabi/ssh-manager/src/v3/internal/util/fs" "github.com/simtabi/ssh-manager/src/v3/internal/util/log" "github.com/simtabi/ssh-manager/src/v3/internal/util/netcheck" @@ -91,11 +92,12 @@ type Rotator struct { m *manifest.Manifest inv *inventory.Inventory ks *keystore.KeyStore + op *opstore.Client // for profiles whose key_store is 1password } // New builds a Rotator. func New(p paths.Paths, m *manifest.Manifest, inv *inventory.Inventory) *Rotator { - return &Rotator{p: p, m: m, inv: inv, ks: keystore.New()} + return &Rotator{p: p, m: m, inv: inv, ks: keystore.New(), op: opstore.New()} } // profileAndHosts resolves a key selector to its owning profile and the hosts @@ -130,6 +132,7 @@ func (r *Rotator) target(h manifest.Host, profile, pubPath, pubText, identPath s Alias: h.Alias, Hostname: h.Hostname, User: h.User, Port: h.Port, PubkeyPath: pubPath, PubkeyText: pubText, TokenEnv: deref(h.TokenEnv), IdentityPath: identPath, KnownHosts: filepath.Join(r.p.SSHDir, "known_hosts"), + IdentityAgent: r.agentFor(profile, identPath), } } @@ -154,6 +157,9 @@ func (r *Rotator) Rotate(selector string, allowUnverified bool, passphrase strin if err != nil { return RotateReport{}, err } + if r.m.Profiles[profile].InOnePassword() { + return r.rotateOnePassword(profile, keyName, hosts, allowUnverified) + } pdir := r.dir(profile) curPriv := filepath.Join(pdir, keyName) curPub := curPriv + ".pub" @@ -339,6 +345,9 @@ func (r *Rotator) Rollback(selector string) (RotateReport, error) { if err != nil { return RotateReport{}, err } + if r.m.Profiles[profile].InOnePassword() { + return r.rollbackOnePassword(profile, keyName, hosts) + } pdir := r.dir(profile) oldPriv := filepath.Join(pdir, "old", keyName) oldPub := oldPriv + ".pub" @@ -385,6 +394,22 @@ func (r *Rotator) Rollback(selector string) (RotateReport, error) { _ = perms.SetPerms(curPriv, perms.PrivateKeyMode) _ = perms.SetPerms(curPub, perms.PublicKeyMode) + return r.finishRollback(profile, keyName, hosts, curPub, curPriv, curPubText, report) +} + +// preservePair copies the keypair at src to dst without disturbing src. +// +// A hard link, not a copy: the two names share one inode, so the source keeps +// working while the archive exists and no private key bytes are written a second +// time. src and dst are both inside one profile directory, so a link is +// essentially always possible; a filesystem that refuses one (a bind mount over +// old/, say) falls back to copying, since preserving the outgoing key matters +// more than how it is preserved. +// finishRollback re-deploys the restored key to every target, revokes the +// rolled-back one, and settles the inventory, once the local files hold the +// restored key. identPath is what a verify login uses. +func (r *Rotator) finishRollback(profile, keyName string, hosts []manifest.Host, + curPub, curPriv, curPubText string, report RotateReport) (RotateReport, error) { restoredPubText := readFile(curPub) var results []TargetResult var unreached []string @@ -437,14 +462,6 @@ func (r *Rotator) Rollback(selector string) (RotateReport, error) { return report, nil } -// preservePair copies the keypair at src to dst without disturbing src. -// -// A hard link, not a copy: the two names share one inode, so the source keeps -// working while the archive exists and no private key bytes are written a second -// time. src and dst are both inside one profile directory, so a link is -// essentially always possible; a filesystem that refuses one (a bind mount over -// old/, say) falls back to copying, since preserving the outgoing key matters -// more than how it is preserved. func preservePair(srcPriv, dstPriv string) error { for _, ext := range []string{"", ".pub"} { src, dst := srcPriv+ext, dstPriv+ext diff --git a/src/internal/services/validate/validate.go b/src/internal/services/validate/validate.go index b64058d..1b66bf1 100644 --- a/src/internal/services/validate/validate.go +++ b/src/internal/services/validate/validate.go @@ -4,6 +4,7 @@ package validate import ( + "errors" "fmt" "os" "strings" @@ -11,6 +12,7 @@ import ( "github.com/simtabi/ssh-manager/src/v3/internal/core/authkeys" "github.com/simtabi/ssh-manager/src/v3/internal/core/manifest" "github.com/simtabi/ssh-manager/src/v3/internal/services/keystore" + "github.com/simtabi/ssh-manager/src/v3/internal/services/opstore" "github.com/simtabi/ssh-manager/src/v3/internal/util/perms" ) @@ -29,11 +31,12 @@ type Service struct { m *manifest.Manifest sshDir string ks *keystore.KeyStore + op *opstore.Client } // New builds a validate service over the given manifest and ~/.ssh directory. func New(m *manifest.Manifest, sshDir string) *Service { - return &Service{m: m, sshDir: sshDir, ks: keystore.New()} + return &Service{m: m, sshDir: sshDir, ks: keystore.New(), op: opstore.New()} } // ValidateKeys validates every managed key, one check per profile+name pair. @@ -74,9 +77,20 @@ func (s *Service) validateOne(profile, keyName, priv string) KeyCheck { var fp *string pubText := "" - if !exists(priv) { + inOP := s.m.Profiles[profile].InOnePassword() + switch { + case inOP && exists(priv): + // ssh is pointed at the .pub and the 1Password agent; a private key file + // here is not what it offers, and is usually the key from before the + // profile moved to 1Password. + issues = append(issues, "a private key file is on disk, but this profile keeps its keys in "+ + "1Password - ssh will not use it. Run `sshmgr rotate "+profile+"/"+keyName+ + "` to generate the key in 1Password") + case inOP: + // expected: the private key is in 1Password + case !exists(priv): issues = append(issues, "private key missing") - } else if !perms.PermsOK(priv, perms.PrivateKeyMode) { + case !perms.PermsOK(priv, perms.PrivateKeyMode): issues = append(issues, "private key perms not 600") } @@ -95,8 +109,13 @@ func (s *Service) validateOne(profile, keyName, priv string) KeyCheck { } } + if inOP { + i, n := s.checkOnePassword(profile, keyName, pubText) + issues, notes = append(issues, i...), append(notes, n...) + } + // Real pair check: derive the public key from the private material. - if exists(priv) { + if !inOP && exists(priv) { derived, encrypted, _ := s.ks.PublicFromPrivate(priv) switch { case derived != "": @@ -116,6 +135,29 @@ func (s *Service) validateOne(profile, keyName, priv string) KeyCheck { } } +// checkOnePassword confirms the key's item is in the profile's vault and that +// its public key is the one on disk - the pair check for a key whose private +// half never leaves 1Password. +func (s *Service) checkOnePassword(profile, keyName, pubText string) (issues, notes []string) { + vault := s.m.Profiles[profile].OPVault + if err := s.op.Available(); err != nil { + return nil, []string{"1Password item not checked: " + err.Error()} + } + it, err := s.op.Get(vault, keyName) + switch { + case errors.Is(err, opstore.ErrNotFound): + return []string{fmt.Sprintf("no item %q in 1Password vault %q - run `sshmgr reconcile` "+ + "to create it", keyName, vault)}, nil + case err != nil: + return nil, []string{"1Password item not checked: " + err.Error()} + } + if pubText != "" && authkeys.KeyBody(it.PublicKey) != authkeys.KeyBody(pubText) { + issues = append(issues, fmt.Sprintf("the .pub does NOT match 1Password item %q in vault %q", + keyName, vault)) + } + return issues, nil +} + func exists(path string) bool { _, err := os.Stat(path) return err == nil diff --git a/src/internal/services/validate/validate_test.go b/src/internal/services/validate/validate_test.go index 16df2f4..1c92ba0 100644 --- a/src/internal/services/validate/validate_test.go +++ b/src/internal/services/validate/validate_test.go @@ -288,3 +288,55 @@ func TestADeclaredButUnwiredKeyIsStillChecked(t *testing.T) { t.Errorf("an unminted declared key should report as missing: %+v", checks[0]) } } + +// A profile moved to 1Password whose old private key file is still on disk is +// the trap: the .pub makes it look present, but ssh is pointed at the 1Password +// agent, which does not hold that key. Validate must say so. +func TestOnePasswordProfileWithAPrivateKeyFileOnDiskFails(t *testing.T) { + if _, err := exec.LookPath("ssh-keygen"); err != nil { + t.Skip("ssh-keygen not on PATH") + } + var m manifest.Manifest + if err := json.Unmarshal([]byte(`{"version":1,"profiles":{"bot":{"key_store":"1password","op_vault":"Simtabi", + "hosts":[{"alias":"gh","hostname":"github.com","user":"git","key_name":"bot_gh-ed25519"}]}}}`), &m); err != nil { + t.Fatal(err) + } + ssh := t.TempDir() + priv := m.KeyPath(ssh, manifest.KeyRef{Profile: "bot", KeyName: "bot_gh-ed25519"}) + if err := os.MkdirAll(filepath.Dir(priv), 0o700); err != nil { + t.Fatal(err) + } + if _, err := keystore.New().Generate(priv, "ed25519", "t", "", false); err != nil { + t.Fatal(err) + } + // No op on PATH (ssh-keygen kept): the vault check degrades to a note. + t.Setenv("PATH", filepath.Dir(mustLook(t, "ssh-keygen"))) + checks, err := New(&m, ssh).ValidateKeys("") + if err != nil { + t.Fatal(err) + } + if len(checks) != 1 || checks[0].OK || !strings.Contains(strings.Join(checks[0].Issues, "|"), "sshmgr rotate") { + t.Fatalf("checks = %+v, want a failing check pointing at rotate", checks) + } + if !strings.Contains(strings.Join(checks[0].Notes, "|"), "not checked") { + t.Errorf("without op the vault check should be a note, got %+v", checks[0]) + } + + // Remove the stray private key: now only the expected public half remains. + if err := os.Remove(priv); err != nil { + t.Fatal(err) + } + checks, _ = New(&m, ssh).ValidateKeys("") + if !checks[0].OK { + t.Errorf("public half only, op unavailable: want OK with a note, got %+v", checks[0]) + } +} + +func mustLook(t *testing.T, name string) string { + t.Helper() + p, err := exec.LookPath(name) + if err != nil { + t.Skip(name + " not on PATH") + } + return p +} diff --git a/src/internal/util/secrets/secrets.go b/src/internal/util/secrets/secrets.go index 528ab3a..6cc4744 100644 --- a/src/internal/util/secrets/secrets.go +++ b/src/internal/util/secrets/secrets.go @@ -1,6 +1,7 @@ // Package secrets resolves provider tokens. A token // value of cmd: runs the command at use-time and uses its trimmed stdout -// as the secret (so it integrates with any secret manager); anything else is +// as the secret (so it integrates with any secret manager); op://vault/item/field +// is a 1Password secret reference, read with `op read`; anything else is // returned as-is. Results are memoized per process. package secrets @@ -10,7 +11,10 @@ import ( "sync" ) -const cmdPrefix = "cmd:" +const ( + cmdPrefix = "cmd:" + opPrefix = "op://" +) // cache memoizes successful lookups only. A cmd: secret typically prompts - a // password manager unlock, a touch on a hardware token - so re-running it for @@ -26,10 +30,18 @@ func Resolve(raw string) string { if raw == "" { return "" } - if !strings.HasPrefix(raw, cmdPrefix) { + var command string + switch { + case strings.HasPrefix(raw, cmdPrefix): + command = raw[len(cmdPrefix):] + case strings.HasPrefix(raw, opPrefix): + // The same as cmd:op read , without asking the user to spell out a + // command. Quoted so a reference with spaces ("op://Simtabi/bot token/ + // credential") stays one argument. + command = "op read " + shellQuote(raw) + default: return raw } - command := raw[len(cmdPrefix):] if v, ok := cache.Load(command); ok { return v.(string) } @@ -94,3 +106,8 @@ func shlexSplit(s string) []string { } return args } + +// shellQuote wraps s in single quotes for shlexSplit, escaping any it contains. +func shellQuote(s string) string { + return "'" + strings.ReplaceAll(s, "'", `'\''`) + "'" +} diff --git a/src/internal/util/secrets/secrets_test.go b/src/internal/util/secrets/secrets_test.go index 7e94b9a..e11d162 100644 --- a/src/internal/util/secrets/secrets_test.go +++ b/src/internal/util/secrets/secrets_test.go @@ -155,3 +155,21 @@ func runCount(t *testing.T, path string) int { } return len(b) } + +// A 1Password secret reference is read with `op read`, the whole reference as +// one argument even when an item or field name has a space in it. +func TestAnOpReferenceIsReadThroughOp(t *testing.T) { + log := stubCommand(t, "op", "printf 'bot-token\\n'\n") + ref := "op://Simtabi/simtabi-bot github/token" + + if got := Resolve(ref); got != "bot-token" { + t.Fatalf("op:// secret = %q, want the stub's output", got) + } + argv, err := os.ReadFile(log) + if err != nil { + t.Fatal(err) + } + if want := "[read][" + ref + "]"; !strings.Contains(strings.ReplaceAll(string(argv), "\n", ""), want) { + t.Errorf("argv = %q, want %s", argv, want) + } +}