From d557a9c33707707e34e0706e19fd191fb3833ad0 Mon Sep 17 00:00:00 2001 From: Imani Manyara Date: Mon, 19 Oct 2026 06:57:02 -0400 Subject: [PATCH 01/17] Add key_store and op_vault to manifest profiles A profile's private keys live on disk (key_store omitted, or "file") or as SSH Key items in a 1Password vault ("1password", which requires op_vault). Any other value is rejected, so a typo cannot fall back to looking for keys on disk. defaults.onepassword_agent optionally overrides the platform's 1Password agent socket. Existing manifests round-trip unchanged: both fields are omitted when unset. --- src/internal/core/manifest/manifest.go | 117 +++++++++++++++++++++++-- 1 file changed, 110 insertions(+), 7 deletions(-) diff --git a/src/internal/core/manifest/manifest.go b/src/internal/core/manifest/manifest.go index ea62610..24c88b4 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,78 @@ 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 (m *Manifest) OnePasswordAgentSocket() string { + if m.Defaults.OnePasswordAgent != "" { + return m.Defaults.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" + } +} + +// InOnePassword reports whether a profile keeps its private keys in 1Password. +func (m *Manifest) InOnePassword(profile string) bool { + return m.Profiles[profile].KeyStore == KeyStoreOnePassword +} + +// UsesOnePassword reports whether any profile keeps its keys in 1Password. +func (m *Manifest) UsesOnePassword() 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 +750,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 @@ -815,10 +905,23 @@ 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 ssh is pointed at RenderedIdentityFile 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 } +// RenderedIdentityFile is the IdentityFile written into ssh_config: the private +// key path, or for a 1Password profile its public key, which is how ssh picks +// one key out of an agent (1Password's documented fix for the six-key limit). +func (m *Manifest) RenderedIdentityFile(profileName, keyName string) string { + if m.InOnePassword(profileName) { + return m.IdentityFile(profileName, keyName) + ".pub" + } + return m.IdentityFile(profileName, keyName) +} + // KeyRef names one key: the profile that owns it plus the file name inside that // profile's directory. Key names are unique per profile, not globally - a person // working under two orgs uses the same file name in both profile directories. From a2c36845a2ed43267ca4ffb69dd0c89949a4957d Mon Sep 17 00:00:00 2001 From: Imani Manyara Date: Mon, 19 Oct 2026 06:57:03 -0400 Subject: [PATCH 02/17] Add a 1Password client and op:// secret references opstore drives the op CLI: create an SSH Key item (the private key is generated inside 1Password and never read back), get an item by title, and retitle or archive it by ID so an edit cannot land on another item that shares a title. Items are only ever archived, never deleted. secrets.Resolve reads op://vault/item/field references with op read, beside the existing cmd: form, so a provider token can live in 1Password without spelling out a command. --- src/internal/services/opstore/opstore.go | 170 ++++++++++++++++++ src/internal/services/opstore/opstore_test.go | 95 ++++++++++ src/internal/util/secrets/secrets.go | 25 ++- src/internal/util/secrets/secrets_test.go | 18 ++ 4 files changed, 304 insertions(+), 4 deletions(-) create mode 100644 src/internal/services/opstore/opstore.go create mode 100644 src/internal/services/opstore/opstore_test.go diff --git a/src/internal/services/opstore/opstore.go b/src/internal/services/opstore/opstore.go new file mode 100644 index 0000000..d6f4b75 --- /dev/null +++ b/src/internal/services/opstore/opstore.go @@ -0,0 +1,170 @@ +// 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 +} + +func isNotFound(err error) bool { + msg := strings.ToLower(err.Error()) + return strings.Contains(msg, "isn't an item") || strings.Contains(msg, "not found") || + strings.Contains(msg, "no 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..b07e0ed --- /dev/null +++ b/src/internal/services/opstore/opstore_test.go @@ -0,0 +1,95 @@ +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) + } + } +} 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) + } +} From da6d6bc7da8188f39dbdd9b62343dae8bff1f9d7 Mon Sep 17 00:00:00 2001 From: Imani Manyara Date: Mon, 19 Oct 2026 07:01:01 -0400 Subject: [PATCH 03/17] Move rollback's redeploy-and-revoke tail into finishRollback A verbatim move with no change in behaviour, so a second rollback path can share it. --- src/internal/services/rotator/rotator.go | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/src/internal/services/rotator/rotator.go b/src/internal/services/rotator/rotator.go index 5f581c1..eac2976 100644 --- a/src/internal/services/rotator/rotator.go +++ b/src/internal/services/rotator/rotator.go @@ -385,6 +385,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 +453,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 From 830aaa8f39db96049ca4ff1923f94047d4eb8707 Mon Sep 17 00:00:00 2001 From: Imani Manyara Date: Mon, 19 Oct 2026 07:06:28 -0400 Subject: [PATCH 04/17] Render 1Password hosts with their public key and the agent A host whose profile keeps its keys in 1Password renders IdentityFile as the key's .pub plus IdentityAgent pointing at the 1Password agent, quoted because the macOS socket path contains a space. A public key in IdentityFile with IdentitiesOnly is how ssh picks one key out of an agent, and is 1Password's documented fix for the six-key limit. An IdentityAgent the host sets itself wins. ssh -G is run on the output to pin that the quoted path parses as one value. The helpers sit on Profile and Defaults rather than Manifest, which is at its method ceiling. --- src/internal/core/manifest/category_test.go | 41 +++++++++++++ src/internal/core/manifest/manifest.go | 30 ++++------ src/internal/core/renderer/category_test.go | 59 +++++++++++++++++++ src/internal/core/renderer/renderer.go | 64 +++++++++++++++------ 4 files changed, 158 insertions(+), 36 deletions(-) diff --git a/src/internal/core/manifest/category_test.go b/src/internal/core/manifest/category_test.go index ee05df8..98105f0 100644 --- a/src/internal/core/manifest/category_test.go +++ b/src/internal/core/manifest/category_test.go @@ -130,3 +130,44 @@ 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) + } +} diff --git a/src/internal/core/manifest/manifest.go b/src/internal/core/manifest/manifest.go index 24c88b4..032056e 100644 --- a/src/internal/core/manifest/manifest.go +++ b/src/internal/core/manifest/manifest.go @@ -680,9 +680,9 @@ func checkKeyStores(m *Manifest) error { // 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 (m *Manifest) OnePasswordAgentSocket() string { - if m.Defaults.OnePasswordAgent != "" { - return m.Defaults.OnePasswordAgent +func (d Defaults) OnePasswordAgentSocket() string { + if d.OnePasswordAgent != "" { + return d.OnePasswordAgent } return onePasswordAgentFor(runtime.GOOS) } @@ -698,13 +698,13 @@ func onePasswordAgentFor(goos string) string { } } -// InOnePassword reports whether a profile keeps its private keys in 1Password. -func (m *Manifest) InOnePassword(profile string) bool { - return m.Profiles[profile].KeyStore == KeyStoreOnePassword -} +// 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. -func (m *Manifest) UsesOnePassword() bool { +// 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 @@ -906,22 +906,12 @@ 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 ssh is pointed at RenderedIdentityFile instead; inventory records and +// 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 } -// RenderedIdentityFile is the IdentityFile written into ssh_config: the private -// key path, or for a 1Password profile its public key, which is how ssh picks -// one key out of an agent (1Password's documented fix for the six-key limit). -func (m *Manifest) RenderedIdentityFile(profileName, keyName string) string { - if m.InOnePassword(profileName) { - return m.IdentityFile(profileName, keyName) + ".pub" - } - return m.IdentityFile(profileName, keyName) -} - // KeyRef names one key: the profile that owns it plus the file name inside that // profile's directory. Key names are unique per profile, not globally - a person // working under two orgs uses the same file name in both profile directories. 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 } From 9d5cd9ecd5e8445353674955ece089c17971a640 Mon Sep 17 00:00:00 2001 From: Imani Manyara Date: Mon, 19 Oct 2026 07:06:28 -0400 Subject: [PATCH 05/17] Treat a 1Password key as present when its public half is Only the .pub of a 1Password key is ever on disk, so key listing, audit, diff and show no longer report it as half a pair. validate checks the vault item exists and matches the .pub, and fails a 1Password profile that still has a private key file on disk: the .pub makes that key look present while ssh asks the 1Password agent, which does not hold it. Agent load skips 1Password keys, and deleting one leaves its vault item in place and says so. --- src/internal/cli/diff.go | 7 ++- src/internal/cli/show.go | 4 ++ src/internal/services/agent/agent.go | 3 ++ src/internal/services/keyaudit/keyaudit.go | 12 ++++- .../services/keyaudit/keyaudit_test.go | 18 +++++++ src/internal/services/keysvc/keysvc.go | 22 +++++++- src/internal/services/lifecycle/lifecycle.go | 13 +++++ src/internal/services/validate/validate.go | 50 ++++++++++++++++-- .../services/validate/validate_test.go | 52 +++++++++++++++++++ 9 files changed, 172 insertions(+), 9 deletions(-) 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/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/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/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..c06f64b 100644 --- a/src/internal/services/keysvc/keysvc.go +++ b/src/internal/services/keysvc/keysvc.go @@ -42,8 +42,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 +173,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 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/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 +} From 65a71571bcffa4b0cb5305f8e9ee84861aa26b8c Mon Sep 17 00:00:00 2001 From: Imani Manyara Date: Mon, 19 Oct 2026 07:06:28 -0400 Subject: [PATCH 06/17] Verify a GitHub key authenticates as the token's account A deploy adds the key to whichever account token_env's token belongs to, so a personal token left in a machine user's variable added it to the wrong account and still passed the key-is-listed check. When the API login and the ssh greeting are both available they must now agree. Verify logins also pass IdentityAgent for a key held by an agent. --- src/internal/core/providers/adapter.go | 16 ++++- src/internal/core/providers/greeting_test.go | 66 ++++++++++++++++++++ src/internal/core/providers/ssh_generic.go | 5 +- src/internal/core/providers/vcs.go | 60 ++++++++++++++++-- 4 files changed, 138 insertions(+), 9 deletions(-) create mode 100644 src/internal/core/providers/greeting_test.go 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 { From 88650ef4fa54261a415a13db2e16a4a7c3106fc0 Mon Sep 17 00:00:00 2001 From: Imani Manyara Date: Mon, 19 Oct 2026 07:06:28 -0400 Subject: [PATCH 07/17] Mint and rotate keys inside 1Password reconcile creates a 1Password profile's key as an SSH Key item titled with the key name and writes only its public half; an existing item of that title is reused, never duplicated. rotate stages the new key as .staged in the vault, deploys and verifies it through the 1Password agent, then retitles it into place, the outgoing item becoming .old and any earlier predecessor going to the vault's Archive. A failed step puts back what had moved. When the outgoing key is still a key file, the same rotation moves the profile into 1Password. rollback restores .old and archives the rotated-in item. Items are archived, never deleted. --- .../services/reconciler/reconciler.go | 70 ++++- src/internal/services/rotator/onepassword.go | 269 ++++++++++++++++++ .../services/rotator/onepassword_test.go | 213 ++++++++++++++ src/internal/services/rotator/rotator.go | 11 +- 4 files changed, 556 insertions(+), 7 deletions(-) create mode 100644 src/internal/services/rotator/onepassword.go create mode 100644 src/internal/services/rotator/onepassword_test.go 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..884cb97 --- /dev/null +++ b/src/internal/services/rotator/onepassword.go @@ -0,0 +1,269 @@ +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) + } + 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) + } + } + 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{}, 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{}, 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{}, err + } + if err := r.op.Rename(vault, staged.ID, keyName); err != nil { + rollbackCommit() + discard() + return RotateReport{}, 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{}, 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{}, 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..21bf496 --- /dev/null +++ b/src/internal/services/rotator/onepassword_test.go @@ -0,0 +1,213 @@ +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 [ "$1" = "--title" ] && echo "$2" > "$V/items/$id/title"; 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) + } +} diff --git a/src/internal/services/rotator/rotator.go b/src/internal/services/rotator/rotator.go index eac2976..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" From 67f44cb8e411447b5314f89bbbb64566095a417b Mon Sep 17 00:00:00 2001 From: Imani Manyara Date: Mon, 19 Oct 2026 07:06:29 -0400 Subject: [PATCH 08/17] Check the op CLI and the 1Password agent in doctor When any profile keeps its keys in 1Password, doctor fails if op is not on PATH or the agent socket is missing, naming the 1Password setting that turns the agent on. --- src/internal/services/doctor/doctor.go | 36 ++++++++++++++++++- .../services/doctor/onepassword_test.go | 24 +++++++++++++ 2 files changed, 59 insertions(+), 1 deletion(-) create mode 100644 src/internal/services/doctor/onepassword_test.go diff --git a/src/internal/services/doctor/doctor.go b/src/internal/services/doctor/doctor.go index 1a0ea72..d9e21a8 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,37 @@ 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") + } + } + 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) + } +} From c1d715c20f69f3bbc9c44fe13f02a55c4877ee36 Mon Sep 17 00:00:00 2001 From: Imani Manyara Date: Mon, 19 Oct 2026 07:09:47 -0400 Subject: [PATCH 09/17] Document 1Password key stores and GitHub machine users Key stores in the configuration reference, the 1Password flow and the machine-user check in rotate, validate and doctor, the rationale in architecture, and two recipes: keeping keys in 1Password, and giving an organization one rotatable bot key through a machine user (a deploy key cannot be shared across repositories). --- CHANGELOG.md | 15 ++++++ README.md | 2 + docs/architecture.md | 15 ++++++ docs/configuration.md | 37 ++++++++++++++- docs/features.md | 2 +- docs/recipes/github-machine-user.md | 61 +++++++++++++++++++++++++ docs/recipes/keep-keys-in-1password.md | 63 ++++++++++++++++++++++++++ docs/tools/doctor.md | 1 + docs/tools/rotate.md | 33 ++++++++++++++ docs/tools/validate.md | 6 +++ 10 files changed, 233 insertions(+), 2 deletions(-) create mode 100644 docs/recipes/github-machine-user.md create mode 100644 docs/recipes/keep-keys-in-1password.md diff --git a/CHANGELOG.md b/CHANGELOG.md index f9d112e..a42d9ff 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,17 @@ 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` and the agent socket. +- **`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 +71,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..26bf00b 100644 --- a/README.md +++ b/README.md @@ -87,6 +87,8 @@ 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. 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..6a80487 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,34 @@ 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 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. + +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 +253,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..e22c70f 100644 --- a/docs/features.md +++ b/docs/features.md @@ -95,7 +95,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..9d3b528 --- /dev/null +++ b/docs/recipes/keep-keys-in-1password.md @@ -0,0 +1,63 @@ +# 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). + +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 +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/tools/doctor.md b/docs/tools/doctor.md index a63c396..ce83e57 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 and the agent socket exists | | `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..176a62f 100644 --- a/docs/tools/rotate.md +++ b/docs/tools/rotate.md @@ -48,6 +48,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..c2148af 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 From c98829b7e8afe88f6d657e3980117138c6e6f5f6 Mon Sep 17 00:00:00 2001 From: Imani Manyara Date: Mon, 19 Oct 2026 07:37:01 -0400 Subject: [PATCH 10/17] Rotate a batch of keys: several, a profile, a category, or all rotate takes any number of keys, profiles or host aliases, or --category or --all, through one selection shared with other bulk commands: a selector that matches nothing is an error, and --all cannot be combined with others. A batch asks once, writes one backup and one snapshot, then rotates each key on its own, saving the inventory after every committed key; a key that fails is left as it was while the rest continue. Keys no host uses are skipped. --dry-run lists the batch. A single key rotates exactly as before. --- src/internal/cli/mutate_test.go | 38 ++++ src/internal/cli/rotate.go | 196 +++++++++++++++----- src/internal/cli/surface_test.go | 6 + src/internal/services/keysvc/keysvc.go | 65 +++++++ src/internal/services/keysvc/keysvc_test.go | 45 +++++ 5 files changed, 304 insertions(+), 46 deletions(-) diff --git a/src/internal/cli/mutate_test.go b/src/internal/cli/mutate_test.go index b5eb8f6..001461c 100644 --- a/src/internal/cli/mutate_test.go +++ b/src/internal/cli/mutate_test.go @@ -517,3 +517,41 @@ 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") + } +} 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/surface_test.go b/src/internal/cli/surface_test.go index bee095e..75a3668 100644 --- a/src/internal/cli/surface_test.go +++ b/src/internal/cli/surface_test.go @@ -100,6 +100,12 @@ 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", + "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/services/keysvc/keysvc.go b/src/internal/services/keysvc/keysvc.go index c06f64b..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" @@ -237,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) + } +} From a4536c8c9251a3ab7553753b38f79e7cffdc3815 Mon Sep 17 00:00:00 2001 From: Imani Manyara Date: Mon, 19 Oct 2026 07:37:01 -0400 Subject: [PATCH 11/17] Add key regen-pub: rebuild public keys from their source key regen-pub rebuilds each selected key's .pub from the private key (ssh-keygen -y) or, for a 1Password profile, from the vault item. It reports each key as unchanged, created or rewritten, keeps the old .pub's comment, and moves the inventory record to the regenerated fingerprint. A .pub that was a different key is called out with a pointer to redeploy. A passphrase-protected key or a missing private key is skipped with the reason. It never changes a private key. --- src/internal/cli/key.go | 62 ++++++ src/internal/cli/mutate_test.go | 23 +++ src/internal/cli/surface_test.go | 5 + src/internal/services/pubkeys/pubkeys.go | 186 ++++++++++++++++++ src/internal/services/pubkeys/pubkeys_test.go | 149 ++++++++++++++ 5 files changed, 425 insertions(+) create mode 100644 src/internal/services/pubkeys/pubkeys.go create mode 100644 src/internal/services/pubkeys/pubkeys_test.go 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 001461c..a0b06d1 100644 --- a/src/internal/cli/mutate_test.go +++ b/src/internal/cli/mutate_test.go @@ -555,3 +555,26 @@ func TestRotateAllRotatesEveryWiredKey(t *testing.T) { 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/surface_test.go b/src/internal/cli/surface_test.go index 75a3668..aaf2b83 100644 --- a/src/internal/cli/surface_test.go +++ b/src/internal/cli/surface_test.go @@ -105,6 +105,11 @@ var goOnly = map[string]string{ "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/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) + } +} From df7232ae1877b54fc601287d2b83be613923309d Mon Sep 17 00:00:00 2001 From: Imani Manyara Date: Mon, 19 Oct 2026 07:37:56 -0400 Subject: [PATCH 12/17] Document batch rotation and key regen-pub A batch section in the rotate reference, key regen-pub beside the other key integrity checks in validate, the feature table, a recipe for rotating or repairing many keys at once, and the changelog. --- CHANGELOG.md | 7 ++++++ README.md | 1 + docs/features.md | 2 ++ docs/recipes/rotate-many-keys.md | 39 ++++++++++++++++++++++++++++++++ docs/tools/rotate.md | 22 ++++++++++++++++++ docs/tools/validate.md | 23 +++++++++++++++++++ 6 files changed, 94 insertions(+) create mode 100644 docs/recipes/rotate-many-keys.md diff --git a/CHANGELOG.md b/CHANGELOG.md index a42d9ff..f4e0d4d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,13 @@ adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). 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` and the agent socket. +- **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": diff --git a/README.md b/README.md index 26bf00b..cd08385 100644 --- a/README.md +++ b/README.md @@ -92,6 +92,7 @@ else is in the [documentation index](#documentation). - [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/features.md b/docs/features.md index e22c70f..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 | 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/rotate.md b/docs/tools/rotate.md index 176a62f..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** diff --git a/docs/tools/validate.md b/docs/tools/validate.md index c2148af..34d556c 100644 --- a/docs/tools/validate.md +++ b/docs/tools/validate.md @@ -36,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 From ffdf17fc6e0d942a3a774c0daf864fea0419dccb Mon Sep 17 00:00:00 2001 From: Imani Manyara Date: Mon, 19 Oct 2026 07:43:11 -0400 Subject: [PATCH 13/17] Check that the 1Password agent serves each profile's vault The agent offers only the Personal, Private and Employee vaults until agent.toml exists, and then only what that file lists, even if it is empty. A key in any other vault is never offered to ssh, which shows up as a bare "Permission denied (publickey)". doctor now checks every vault a profile names and prints the [[ssh-keys]] entry to add. It asks for a whole-vault entry because an item-only one would not serve a rotation's staged key. Documented in the configuration reference and the 1Password recipe. --- docs/configuration.md | 14 +++ docs/recipes/keep-keys-in-1password.md | 11 +- docs/tools/doctor.md | 2 +- src/internal/services/doctor/doctor.go | 13 +++ src/internal/services/opstore/agentconfig.go | 101 ++++++++++++++++++ .../services/opstore/agentconfig_test.go | 43 ++++++++ 6 files changed, 182 insertions(+), 2 deletions(-) create mode 100644 src/internal/services/opstore/agentconfig.go create mode 100644 src/internal/services/opstore/agentconfig_test.go diff --git a/docs/configuration.md b/docs/configuration.md index 6a80487..de6c186 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -195,6 +195,20 @@ 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 diff --git a/docs/recipes/keep-keys-in-1password.md b/docs/recipes/keep-keys-in-1password.md index 9d3b528..b3ffead 100644 --- a/docs/recipes/keep-keys-in-1password.md +++ b/docs/recipes/keep-keys-in-1password.md @@ -7,6 +7,15 @@ You need the [1Password CLI](https://developer.1password.com/docs/cli/get-starte (`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 @@ -23,7 +32,7 @@ In the manifest, give the profile a store and a vault: ``` ```sh -sshmgr doctor # op found, agent socket present +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 ``` diff --git a/docs/tools/doctor.md b/docs/tools/doctor.md index ce83e57..a8b5464 100644 --- a/docs/tools/doctor.md +++ b/docs/tools/doctor.md @@ -20,7 +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 and the agent socket exists | +| `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/src/internal/services/doctor/doctor.go b/src/internal/services/doctor/doctor.go index d9e21a8..0ee1ee3 100644 --- a/src/internal/services/doctor/doctor.go +++ b/src/internal/services/doctor/doctor.go @@ -300,6 +300,19 @@ func onePasswordIssues(m *manifest.Manifest) []string { "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 } 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) + } + } +} From 9c099a3a0fc678e26c7960e1f679ca78130fb6a2 Mon Sep 17 00:00:00 2001 From: Imani Manyara Date: Mon, 19 Oct 2026 07:43:50 -0400 Subject: [PATCH 14/17] Reject 1Password key setups that would share or misstate a key A vault item is titled with the bare key name, and key names are only unique within a profile, so two 1Password profiles sharing a vault and a key name would have found each other's item: two identities on one key. 1Password also generates Ed25519 only, so any other key_type on a 1Password profile was silently ignored. Both are now load-time errors. --- docs/configuration.md | 7 ++++ src/internal/core/manifest/category_test.go | 28 ++++++++++++++++ src/internal/core/manifest/manifest.go | 37 ++++++++++++++++++++- 3 files changed, 71 insertions(+), 1 deletion(-) diff --git a/docs/configuration.md b/docs/configuration.md index de6c186..7abc9ed 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -215,6 +215,13 @@ The agent socket defaults to the platform's path (macOS 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 diff --git a/src/internal/core/manifest/category_test.go b/src/internal/core/manifest/category_test.go index 98105f0..cd48a62 100644 --- a/src/internal/core/manifest/category_test.go +++ b/src/internal/core/manifest/category_test.go @@ -171,3 +171,31 @@ func TestOnePasswordAgentSocketPerPlatform(t *testing.T) { 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 032056e..d4f5f2c 100644 --- a/src/internal/core/manifest/manifest.go +++ b/src/internal/core/manifest/manifest.go @@ -698,6 +698,41 @@ func onePasswordAgentFor(goos string) string { } } +// 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 } @@ -788,7 +823,7 @@ func (m *Manifest) validate() error { } } } - return nil + return checkOnePasswordKeys(m) } func (m *Manifest) sortedProfileNames() []string { From 6bdc164f2db2bf7ad0b47eda4d593f37d3dc039d Mon Sep 17 00:00:00 2001 From: Imani Manyara Date: Mon, 19 Oct 2026 07:44:04 -0400 Subject: [PATCH 15/17] Treat only op's "isn't an item" as a missing 1Password item The not-found check matched any error containing "not found" or "no item", so a mistyped vault or a missing account read as a missing item and reconcile tried to create it. Only op's own wording for a missing item counts now; everything else surfaces as the error it is. --- src/internal/services/opstore/opstore.go | 8 +++++--- src/internal/services/opstore/opstore_test.go | 14 ++++++++++++++ 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/src/internal/services/opstore/opstore.go b/src/internal/services/opstore/opstore.go index d6f4b75..618d0e1 100644 --- a/src/internal/services/opstore/opstore.go +++ b/src/internal/services/opstore/opstore.go @@ -122,10 +122,12 @@ func (c *Client) run(args ...string) ([]byte, error) { 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 { - msg := strings.ToLower(err.Error()) - return strings.Contains(msg, "isn't an item") || strings.Contains(msg, "not found") || - strings.Contains(msg, "no item") + return strings.Contains(strings.ToLower(err.Error()), "isn't an item") } // parseItem reads the fields sshmgr needs from `op item ... --format json`. diff --git a/src/internal/services/opstore/opstore_test.go b/src/internal/services/opstore/opstore_test.go index b07e0ed..86bc3cf 100644 --- a/src/internal/services/opstore/opstore_test.go +++ b/src/internal/services/opstore/opstore_test.go @@ -93,3 +93,17 @@ func TestEditAndArchiveAddressTheItemByID(t *testing.T) { } } } + +// 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) + } + } +} From 233bcd4fab949a2da283410587f972740851e921 Mon Sep 17 00:00:00 2001 From: Imani Manyara Date: Mon, 19 Oct 2026 07:45:03 -0400 Subject: [PATCH 16/17] Name the archived predecessor when a 1Password rotation fails Commit archives the earlier .old before retitling. A later failure puts everything else back, but not that archive, and the error said nothing of it. Every failure after that point now says the predecessor was moved to the vault's Archive and can be restored from there. --- src/internal/services/rotator/onepassword.go | 30 +++++++++------ .../services/rotator/onepassword_test.go | 38 ++++++++++++++++++- 2 files changed, 56 insertions(+), 12 deletions(-) diff --git a/src/internal/services/rotator/onepassword.go b/src/internal/services/rotator/onepassword.go index 884cb97..672b99f 100644 --- a/src/internal/services/rotator/onepassword.go +++ b/src/internal/services/rotator/onepassword.go @@ -131,6 +131,10 @@ func (r *Rotator) rotateOnePassword(profile, keyName string, hosts []manifest.Ho 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. @@ -139,6 +143,10 @@ func (r *Rotator) rotateOnePassword(profile, keyName string, hosts []manifest.Ho 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() { @@ -151,8 +159,8 @@ func (r *Rotator) rotateOnePassword(profile, keyName string, hosts []manifest.Ho case err == nil: if err := r.op.Rename(vault, cur.ID, oldTitle(keyName)); err != nil { discard() - return RotateReport{}, fmt.Errorf("could not retitle the outgoing key, so the rotation "+ - "was not committed (the active key is untouched): %w", err) + 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): @@ -160,36 +168,36 @@ func (r *Rotator) rotateOnePassword(profile, keyName string, hosts []manifest.Ho oldPriv := filepath.Join(oldDir, keyName) if err := os.Rename(curPriv, oldPriv); err != nil { discard() - return RotateReport{}, fmt.Errorf("could not archive the outgoing key file, so the "+ - "rotation was not committed: %w", err) + 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{}, err + return RotateReport{}, note(err) } if err := r.op.Rename(vault, staged.ID, keyName); err != nil { rollbackCommit() discard() - return RotateReport{}, fmt.Errorf("could not give the new key its title, so the rotation was "+ - "undone and the previous key is active again: %w", err) + 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{}, fmt.Errorf("could not archive the outgoing public key, so the rotation "+ - "was undone: %w", err) + 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{}, fmt.Errorf("could not move the new public key into place, so the "+ - "rotation was undone: %w", err) + 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) diff --git a/src/internal/services/rotator/onepassword_test.go b/src/internal/services/rotator/onepassword_test.go index 21bf496..74b1623 100644 --- a/src/internal/services/rotator/onepassword_test.go +++ b/src/internal/services/rotator/onepassword_test.go @@ -35,7 +35,11 @@ case "$cmd" in 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 [ "$1" = "--title" ] && echo "$2" > "$V/items/$id/title"; shift; done ;; + 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 ;; @@ -211,3 +215,35 @@ func TestRotateMovesAFileKeyIntoOnePassword(t *testing.T) { 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()) + } +} From 96f4c4fb00008b6f017d037bc5951b5816560198 Mon Sep 17 00:00:00 2001 From: Imani Manyara Date: Mon, 19 Oct 2026 07:46:49 -0400 Subject: [PATCH 17/17] Note the 1Password vault and key rules in the changelog --- CHANGELOG.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f4e0d4d..7d789d6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,7 +16,9 @@ adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). 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` and the agent socket. + `.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