From d2b4b5ae98e47193d9fd56f848908a0395da3492 Mon Sep 17 00:00:00 2001 From: Jason Bowman Date: Thu, 20 Aug 2026 13:01:23 -0700 Subject: [PATCH 01/13] feat: load certificate into ssh-agent on login with configurable lifetime When SSH_AUTH_SOCK points at a running ssh-agent, opkssh login now also adds the freshly minted certificate and its private key to the agent, so the key is usable immediately without pointing ssh at the key files. Best-effort and non-fatal: the keys are still written to disk first, and any agent problem is reported as a warning rather than failing the login. The key is always added with a lifetime, since ssh-agent has no replace operation and keys would otherwise accumulate with every login. The lifetime is resolved as: --lifetime flag, then agent_lifetime in the client config (~/.opk/config.yml), then a default of 24h matching the server's default certificate expiration policy. Both duration strings (12h, 45m) and raw seconds (28800) are accepted, and an invalid value fails before the browser dance rather than after authentication. Relates to #6 and #96; first of the two PRs agreed in #606 (the second adds a refresh daemon that rotates agent keys as tokens are refreshed). --- commands/config/client_config.go | 1 + commands/config/client_config_test.go | 27 +++++ commands/config/default-client-config.yml | 5 + commands/login.go | 124 +++++++++++++++++++- commands/login_test.go | 135 +++++++++++++++++++++- docs/config.md | 2 + main.go | 4 +- 7 files changed, 295 insertions(+), 3 deletions(-) diff --git a/commands/config/client_config.go b/commands/config/client_config.go index e27e03c1..b4333ce7 100644 --- a/commands/config/client_config.go +++ b/commands/config/client_config.go @@ -33,6 +33,7 @@ var DefaultClientConfig []byte type ClientConfig struct { DefaultProvider string `yaml:"default_provider"` Providers []ProviderConfig `yaml:"providers"` + AgentLifetime string `yaml:"agent_lifetime,omitempty"` } func NewClientConfig(c []byte) (*ClientConfig, error) { diff --git a/commands/config/client_config_test.go b/commands/config/client_config_test.go index 04c235b9..cf2ca396 100644 --- a/commands/config/client_config_test.go +++ b/commands/config/client_config_test.go @@ -76,3 +76,30 @@ providers: require.NotNil(t, clientConfig) require.Equal(t, clientConfig.Providers[0].SendAccessToken, true) } + +func TestParseConfigWithAgentLifetime(t *testing.T) { + c := `--- +agent_lifetime: 12h +providers: + - alias: google + issuer: https://accounts.google.com + client_id: test-client-id` + + clientConfig, err := NewClientConfig([]byte(c)) + require.NoError(t, err) + require.NotNil(t, clientConfig) + require.Equal(t, "12h", clientConfig.AgentLifetime) + + // Test with numeric seconds + c2 := `--- +agent_lifetime: 28800 +providers: + - alias: google + issuer: https://accounts.google.com + client_id: test-client-id` + + clientConfig2, err := NewClientConfig([]byte(c2)) + require.NoError(t, err) + require.NotNil(t, clientConfig2) + require.Equal(t, "28800", clientConfig2.AgentLifetime) +} diff --git a/commands/config/default-client-config.yml b/commands/config/default-client-config.yml index 583c280f..48b71300 100644 --- a/commands/config/default-client-config.yml +++ b/commands/config/default-client-config.yml @@ -1,6 +1,11 @@ --- default_provider: webchooser +# How long ssh-agent retains the certificate added by opkssh login, as a +# duration (12h, 45m) or in seconds (28800). Defaults to 24h, matching the +# server's default certificate expiration policy. +# agent_lifetime: 24h + providers: - alias: google issuer: https://accounts.google.com diff --git a/commands/login.go b/commands/login.go index 5bc83c9a..a4f11cf4 100644 --- a/commands/login.go +++ b/commands/login.go @@ -28,11 +28,14 @@ import ( "fmt" "io" "log" + "math" + "net" "net/url" "os" "path/filepath" "regexp" "slices" + "strconv" "strings" "time" @@ -49,6 +52,7 @@ import ( "github.com/thediveo/enumflag/v2" "golang.org/x/crypto/ed25519" "golang.org/x/crypto/ssh" + "golang.org/x/crypto/ssh/agent" ) // KeyType is the algorithm to use for the user's key pair. This is used both by OpenPubkey as algorithm for upk (user public key) and by SSH for public key in the SSH certificate generated by opkssh. @@ -100,6 +104,7 @@ type LoginCmd struct { Verbosity int // Default verbosity is 0, 1 is verbose, 2 is debug RemoteRedirectURI string PrincipalsArg []string // The principals that will be included in the generated SSH cert. If not specified, it functions as a wildcard and will work for any principals. + AgentLifetimeArg string // How long ssh-agent should retain the certificate (--lifetime flag; overrides agent_lifetime in the client config) overrideProvider *providers.OpenIdProvider // Used in tests to override the provider to inject a mock provider // State @@ -119,7 +124,7 @@ type LoginCmd struct { func NewLogin(autoRefreshArg bool, configPathArg string, createConfigArg bool, configureArg bool, logDirArg string, sendAccessTokenArg bool, disableBrowserOpenArg bool, printIdTokenArg bool, providerArg string, printKeyArg bool, keyPathArg string, providerAliasArg string, keyTypeArg KeyType, - remoteRedirectUri string, inspectCertArg bool, principalsArg []string, + remoteRedirectUri string, inspectCertArg bool, principalsArg []string, agentLifetimeArg string, ) *LoginCmd { return &LoginCmd{ Fs: afero.NewOsFs(), @@ -139,6 +144,7 @@ func NewLogin(autoRefreshArg bool, configPathArg string, createConfigArg bool, c KeyTypeArg: keyTypeArg, RemoteRedirectURI: remoteRedirectUri, PrincipalsArg: principalsArg, + AgentLifetimeArg: agentLifetimeArg, } } @@ -199,6 +205,13 @@ func (l *LoginCmd) Run(ctx context.Context) error { l.checkSSHConfigured() } + // Validate the requested ssh-agent lifetime up front so that a typo in + // --lifetime (or agent_lifetime in the client config) fails before the + // browser dance rather than being discovered after authentication. + if _, err := l.resolveAgentLifetimeSecs(); err != nil { + return err + } + if kind, forgejoIssuer := detectActionsEnvironment(); kind == actionsEnvForgejo { l.Config.Providers = append(l.Config.Providers, config.ForgejoProviderConfig(forgejoIssuer)) } else if kind == actionsEnvGithub { @@ -560,6 +573,14 @@ func (l *LoginCmd) login(ctx context.Context, provider providers.OpenIdProvider, return nil, err } + // Also load the certificate and its private key into a running ssh-agent + // (if one is reachable via SSH_AUTH_SOCK). Best-effort and non-fatal: the + // keys were already written to disk above, so a missing or broken agent + // must not fail the login. + if !l.PrintKeyArg { + l.addCertToAgent(certBytes, signer) + } + if printIdToken { idTokenStr, err := PrettyIdToken(*pkt) if err != nil { @@ -675,6 +696,107 @@ func (l *LoginCmd) out() io.Writer { return os.Stdout } +// defaultAgentLifetime is how long ssh-agent retains the certificate when +// neither --lifetime nor agent_lifetime in the client config is set. It matches +// the opkssh server's default certificate expiration policy of 24 hours from +// the ID token's issuance (see docs/config.md), so by default the agent drops +// the key around the time the server stops accepting it. +const defaultAgentLifetime = 24 * time.Hour + +// addCertToAgent loads the freshly minted certificate and its private key into +// the ssh-agent reachable via SSH_AUTH_SOCK. The key is always added with a +// lifetime: ssh-agent has no replace operation, so without one every login +// would accumulate another key in the agent forever. Best-effort: any problem +// is reported and swallowed so login still succeeds (the keys were already +// written to disk before this is called). +func (l *LoginCmd) addCertToAgent(certBytes []byte, signer crypto.Signer) { + sock := os.Getenv("SSH_AUTH_SOCK") + if sock == "" { + return + } + + lifetimeSecs, err := l.resolveAgentLifetimeSecs() + if err != nil { + fmt.Fprintf(l.out(), "warning: not adding certificate to ssh-agent: %v\n", err) + return + } + + pubkey, _, _, _, err := ssh.ParseAuthorizedKey(certBytes) + if err != nil { + fmt.Fprintf(l.out(), "warning: could not parse generated certificate for ssh-agent: %v\n", err) + return + } + cert, ok := pubkey.(*ssh.Certificate) + if !ok { + fmt.Fprintln(l.out(), "warning: generated key is not a certificate; not adding to ssh-agent") + return + } + + conn, err := net.Dial("unix", sock) + if err != nil { + fmt.Fprintf(l.out(), "warning: could not connect to ssh-agent (%s): %v\n", sock, err) + return + } + defer conn.Close() + + if err := agent.NewClient(conn).Add(agent.AddedKey{ + PrivateKey: signer, + Certificate: cert, + Comment: "opkssh", + LifetimeSecs: lifetimeSecs, + }); err != nil { + fmt.Fprintf(l.out(), "warning: failed to add certificate to ssh-agent: %v\n", err) + return + } + fmt.Fprintf(l.out(), "Certificate added to ssh-agent (lifetime %s)\n", time.Duration(lifetimeSecs)*time.Second) +} + +// resolveAgentLifetimeSecs returns how long, in seconds, ssh-agent should +// retain the certificate: the --lifetime flag if set, otherwise agent_lifetime +// from the client config, otherwise defaultAgentLifetime. The client cannot +// know the server's real expiration policy (the server computes it from the ID +// token's iat claim), so this is a client-side bound that keeps keys from +// accumulating in the agent, not a statement of the certificate's validity. +func (l *LoginCmd) resolveAgentLifetimeSecs() (uint32, error) { + var source, value string + switch { + case l.AgentLifetimeArg != "": + source, value = "--lifetime", l.AgentLifetimeArg + case l.Config != nil && l.Config.AgentLifetime != "": + source, value = "agent_lifetime in client config", l.Config.AgentLifetime + default: + return uint32(defaultAgentLifetime / time.Second), nil + } + + d, err := parseDurationOrSeconds(value) + if err != nil { + return 0, fmt.Errorf("invalid %s value %q: %w", source, value, err) + } + if d <= 0 { + return 0, fmt.Errorf("invalid %s value %q: must be positive", source, value) + } + if d > math.MaxUint32*time.Second { + return 0, fmt.Errorf("invalid %s value %q: too large", source, value) + } + return uint32(d / time.Second), nil +} + +// parseDurationOrSeconds parses a lifetime given either as a Go duration +// string (e.g. "8h", "45m") or as a raw number of seconds (e.g. "28800"). +func parseDurationOrSeconds(s string) (time.Duration, error) { + s = strings.TrimSpace(s) + if s == "" { + return 0, errors.New("empty duration") + } + if d, err := time.ParseDuration(s); err == nil { + return d, nil + } + if sec, err := strconv.ParseInt(s, 10, 64); err == nil { + return time.Duration(sec) * time.Second, nil + } + return 0, errors.New("expected a duration like 8h or 45m, or a number of seconds like 28800") +} + func createSSHCert(pkt *pktoken.PKToken, signer crypto.Signer, principals []string) ([]byte, []byte, error) { return createSSHCertWithAccessToken(pkt, nil, signer, principals) } diff --git a/commands/login_test.go b/commands/login_test.go index b37a97a2..c3da2e65 100644 --- a/commands/login_test.go +++ b/commands/login_test.go @@ -23,6 +23,7 @@ import ( "crypto/rand" "encoding/json" "fmt" + "net" "os" "path/filepath" "runtime" @@ -43,6 +44,7 @@ import ( "github.com/spf13/afero" "github.com/stretchr/testify/require" "golang.org/x/crypto/ssh" + "golang.org/x/crypto/ssh/agent" ) const providerAlias1 = "op1" @@ -100,6 +102,9 @@ func Mocks(t *testing.T, keyType KeyType, extraClaims ...map[string]any) (*pktok } func TestLoginCmd(t *testing.T) { + // Keep the login flow away from any real ssh-agent the test runner has. + t.Setenv("SSH_AUTH_SOCK", "") + logDir := "./logs" logPath := filepath.Join(logDir, "opkssh.log") @@ -513,10 +518,132 @@ func TestNewLogin(t *testing.T) { keyTypeArg := ECDSA remoteRedirectURIArg := "" var principalsDesired []string = nil + agentLifetimeArg := "12h" loginCmd := NewLogin(autoRefresh, configPathArg, createConfig, configureArg, logDir, - sendAccessTokenArg, disableBrowserOpenArg, printIdTokenArg, providerArg, keyAsOutputArg, keyPathArg, providerAlias, keyTypeArg, remoteRedirectURIArg, false, principalsDesired) + sendAccessTokenArg, disableBrowserOpenArg, printIdTokenArg, providerArg, keyAsOutputArg, keyPathArg, providerAlias, keyTypeArg, remoteRedirectURIArg, false, principalsDesired, agentLifetimeArg) require.NotNil(t, loginCmd) + require.Equal(t, "12h", loginCmd.AgentLifetimeArg) +} + +func TestResolveAgentLifetimeSecs(t *testing.T) { + tests := []struct { + name string + cmd LoginCmd + expected uint32 + errMsg string + }{ + { + name: "default is 24h when nothing is configured", + cmd: LoginCmd{}, + expected: 86400, + }, + { + name: "--lifetime flag as duration", + cmd: LoginCmd{AgentLifetimeArg: "12h"}, + expected: 12 * 3600, + }, + { + name: "--lifetime flag as raw seconds", + cmd: LoginCmd{AgentLifetimeArg: "28800"}, + expected: 28800, + }, + { + name: "agent_lifetime from client config", + cmd: LoginCmd{Config: &config.ClientConfig{AgentLifetime: "8h"}}, + expected: 8 * 3600, + }, + { + name: "--lifetime flag overrides client config", + cmd: LoginCmd{AgentLifetimeArg: "2h", Config: &config.ClientConfig{AgentLifetime: "8h"}}, + expected: 2 * 3600, + }, + { + name: "invalid flag value is an error", + cmd: LoginCmd{AgentLifetimeArg: "tomorrow"}, + errMsg: "invalid --lifetime value", + }, + { + name: "zero flag value is an error", + cmd: LoginCmd{AgentLifetimeArg: "0"}, + errMsg: "must be positive", + }, + { + name: "negative flag value is an error", + cmd: LoginCmd{AgentLifetimeArg: "-5m"}, + errMsg: "must be positive", + }, + { + name: "invalid config value is an error", + cmd: LoginCmd{Config: &config.ClientConfig{AgentLifetime: "not-a-duration"}}, + errMsg: "invalid agent_lifetime in client config", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + secs, err := tt.cmd.resolveAgentLifetimeSecs() + if tt.errMsg != "" { + require.ErrorContains(t, err, tt.errMsg) + return + } + require.NoError(t, err) + require.Equal(t, tt.expected, secs) + }) + } +} + +// recordingAgent wraps an in-memory ssh-agent and records the keys added to +// it, because agent.Keyring does not expose constraints like LifetimeSecs +// back through List. +type recordingAgent struct { + agent.Agent + mu sync.Mutex + added []agent.AddedKey +} + +func (r *recordingAgent) Add(key agent.AddedKey) error { + r.mu.Lock() + r.added = append(r.added, key) + r.mu.Unlock() + return r.Agent.Add(key) +} + +func TestAddCertToAgent(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("test agent listens on a unix domain socket") + } + + pkt, signer, _ := Mocks(t, ECDSA) + certBytes, _, err := createSSHCert(pkt, signer, []string{"test"}) + require.NoError(t, err) + + sockPath := filepath.Join(t.TempDir(), "agent.sock") + listener, err := net.Listen("unix", sockPath) + require.NoError(t, err) + defer listener.Close() + + mockAgent := &recordingAgent{Agent: agent.NewKeyring()} + go func() { + conn, err := listener.Accept() + if err != nil { + return + } + _ = agent.ServeAgent(mockAgent, conn) + }() + + out := &bytes.Buffer{} + t.Setenv("SSH_AUTH_SOCK", sockPath) + l := &LoginCmd{AgentLifetimeArg: "2h", OutWriter: out} + l.addCertToAgent(certBytes, signer) + require.Contains(t, out.String(), "Certificate added to ssh-agent") + + mockAgent.mu.Lock() + defer mockAgent.mu.Unlock() + require.Len(t, mockAgent.added, 1) + added := mockAgent.added[0] + require.NotNil(t, added.Certificate) + require.Equal(t, uint32(2*3600), added.LifetimeSecs) + require.Equal(t, "opkssh", added.Comment) } func TestCreateSSHCert(t *testing.T) { @@ -782,6 +909,9 @@ func TestWriteKeysToDestination(t *testing.T) { // (including its error return). The success side of this call site is also // covered by TestLoginCmd; the error case here covers the `return nil, err`. func TestLoginWritesKeysToDestination(t *testing.T) { + // Keep the login flow away from any real ssh-agent the test runner has. + t.Setenv("SSH_AUTH_SOCK", "") + home, err := os.UserHomeDir() require.NoError(t, err) @@ -898,6 +1028,9 @@ func (c *writeCountingFs) count() int { // through the shared helper. The token is given a short expiry so the first // refresh fires immediately, and a bounded context guarantees the loop exits. func TestLoginWithRefreshWritesKeysToDestination(t *testing.T) { + // Keep the login flow away from any real ssh-agent the test runner has. + t.Setenv("SSH_AUTH_SOCK", "") + // Short expiry: LoginWithRefresh waits until ~1 minute before expiry, so a // near-term exp makes the first refresh fire right away. shortExp := map[string]any{ diff --git a/docs/config.md b/docs/config.md index 867deec2..c65836b0 100644 --- a/docs/config.md +++ b/docs/config.md @@ -25,6 +25,8 @@ The client config can be used to configure the following values: - **default_provider** By default this is set to the webchooser, which opens a webpage and allows the user to select the OpenID Provider they want by clicking. However if you wish to always connect to one particular OpenID Provider you can set this to the alias of that OpenID Provider and it will skip the web chooser and automatically just open a browser window to that provider. +- **agent_lifetime** How long ssh-agent retains the certificate that `opkssh login` adds to it, either as a duration (`12h`, `45m`) or in seconds (`28800`). If unset it defaults to `24h`, matching the server's default certificate expiration policy. Note the client cannot know the expiration policy of every server it connects to, so this is a client-side bound to keep expired keys from accumulating in the agent rather than a statement of the certificate's validity. The `--lifetime` flag on `opkssh login` overrides this value. + - **providers** This allows you to configure all the OpenID Providers you wish to use. See example below. - **send_access_token** Is a boolean value scoped to a particular provider. It determines if opkssh should put the user's access token into the SSH public key (SSH Certificate). This is useful for allowing the opkssh verifier to read claims not available in the ID Token that can only be read from the OpenID Provider's [userinfo endpoint](https://openid.net/specs/openid-connect-core-1_0.html#UserInfo). The opkssh verifier on the SSH server will use the access token to make a call to the OpenID Provider's userinfo endpoint. Configuration option false by default as SSH will send SSH Public Keys to any host you are attempting to SSH into. Before setting this to true carefully consider the security implications of including the access token in the SSH Public key. diff --git a/main.go b/main.go index 998794c3..ec55ff55 100644 --- a/main.go +++ b/main.go @@ -159,6 +159,7 @@ Arguments: var keyTypeArg commands.KeyType var remoteRedirectURIArg string var principalsArg []string + var agentLifetimeArg string loginCmd := &cobra.Command{ SilenceUsage: true, @@ -196,7 +197,7 @@ Arguments: login := commands.NewLogin(autoRefreshArg, configPathArg, createConfigArg, configureArg, logDirArg, sendAccessTokenArg, disableBrowserOpenArg, printIdTokenArg, providerArg, printKeyArg, keyPathArg, - providerAliasArg, keyTypeArg, remoteRedirectURIArg, inspectCertArg, principalsArg) + providerAliasArg, keyTypeArg, remoteRedirectURIArg, inspectCertArg, principalsArg, agentLifetimeArg) if err := login.Run(ctx); err != nil { log.Println("Error executing login command:", err) return err @@ -223,6 +224,7 @@ Arguments: loginCmd.Flags().StringVar(&remoteRedirectURIArg, "remote-redirect-uri", "", "Remote redirect URI used for non-localhost redirects. This is an advanced option for embedding opkssh in server-side logic.") loginCmd.Flags().VarP(enumflag.New(&keyTypeArg, "Key Type", map[commands.KeyType][]string{commands.ECDSA: {commands.ECDSA.String()}, commands.ED25519: {commands.ED25519.String()}}, enumflag.EnumCaseInsensitive), "key-type", "t", "Type of key to generate") loginCmd.Flags().StringSliceVar(&principalsArg, "principals", nil, "Comma separated list of principals to include in the generated SSH certificate. If not specified it will work for any principal. Do not use unless you know what you are doing.") + loginCmd.Flags().StringVar(&agentLifetimeArg, "lifetime", "", "How long ssh-agent retains the certificate when it is added at login, as a duration (e.g. 12h, 45m) or in seconds (e.g. 28800). Overrides agent_lifetime in the client config. Defaults to 24h.") rootCmd.AddCommand(loginCmd) var logoutKeyPathArg string From e862bb94ead0a588266b6716d7689712f949a1a7 Mon Sep 17 00:00:00 2001 From: Jason Bowman Date: Thu, 20 Aug 2026 13:22:18 -0700 Subject: [PATCH 02/13] fix: close lifetime edge cases and harden the agent exchange MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review findings on the agent-lifetime feature: - Reject lifetimes under one second: uint32(d / time.Second) truncated sub-second durations (500ms, 900ms) to LifetimeSecs 0, which the agent protocol treats as no lifetime at all — an immortal key, the exact state the lifetime exists to prevent. Bound raw seconds values before multiplying, since time.Duration(sec) * time.Second wraps int64 for large inputs and could land on a small bogus duration. - Set a 5s deadline on the ssh-agent exchange so a dead socket (e.g. a forwarded agent whose upstream connection is gone) degrades to a warning instead of hanging the login after authentication succeeded. - Unset SSH_AUTH_SOCK in the integration suite's TestMain so its seven login flows can never inject test keys into a developer's real agent. - Document that the agent is not currently reachable on Windows. --- commands/login.go | 21 +++++++++++++++++++-- commands/login_test.go | 28 ++++++++++++++++++++++++++-- docs/config.md | 2 +- test/integration/opkssh_test.go | 6 ++++++ 4 files changed, 52 insertions(+), 5 deletions(-) diff --git a/commands/login.go b/commands/login.go index a4f11cf4..599e1d67 100644 --- a/commands/login.go +++ b/commands/login.go @@ -739,6 +739,14 @@ func (l *LoginCmd) addCertToAgent(certBytes []byte, signer crypto.Signer) { } defer conn.Close() + // Deadline on the whole exchange: a dead socket (e.g. a forwarded agent + // whose upstream connection is gone) accepts the dial but never responds, + // which would otherwise hang the login after authentication succeeded. + if err := conn.SetDeadline(time.Now().Add(5 * time.Second)); err != nil { + fmt.Fprintf(l.out(), "warning: could not set ssh-agent I/O deadline: %v\n", err) + return + } + if err := agent.NewClient(conn).Add(agent.AddedKey{ PrivateKey: signer, Certificate: cert, @@ -772,8 +780,11 @@ func (l *LoginCmd) resolveAgentLifetimeSecs() (uint32, error) { if err != nil { return 0, fmt.Errorf("invalid %s value %q: %w", source, value, err) } - if d <= 0 { - return 0, fmt.Errorf("invalid %s value %q: must be positive", source, value) + // Below one second the conversion would truncate to LifetimeSecs 0, which + // the agent protocol treats as "no lifetime" — an immortal key, the exact + // state the lifetime exists to prevent. + if d < time.Second { + return 0, fmt.Errorf("invalid %s value %q: must be at least 1 second", source, value) } if d > math.MaxUint32*time.Second { return 0, fmt.Errorf("invalid %s value %q: too large", source, value) @@ -792,6 +803,12 @@ func parseDurationOrSeconds(s string) (time.Duration, error) { return d, nil } if sec, err := strconv.ParseInt(s, 10, 64); err == nil { + // Bound before multiplying: time.Duration(sec) * time.Second overflows + // int64 for large values and can wrap around into a small positive + // duration. + if sec < 0 || sec > math.MaxUint32 { + return 0, fmt.Errorf("seconds value %d out of range", sec) + } return time.Duration(sec) * time.Second, nil } return 0, errors.New("expected a duration like 8h or 45m, or a number of seconds like 28800") diff --git a/commands/login_test.go b/commands/login_test.go index c3da2e65..06e7c31a 100644 --- a/commands/login_test.go +++ b/commands/login_test.go @@ -566,12 +566,36 @@ func TestResolveAgentLifetimeSecs(t *testing.T) { { name: "zero flag value is an error", cmd: LoginCmd{AgentLifetimeArg: "0"}, - errMsg: "must be positive", + errMsg: "must be at least 1 second", }, { name: "negative flag value is an error", cmd: LoginCmd{AgentLifetimeArg: "-5m"}, - errMsg: "must be positive", + errMsg: "must be at least 1 second", + }, + { + // Sub-second values would truncate to LifetimeSecs 0, which the + // agent protocol treats as no lifetime at all. + name: "sub-second flag value is an error", + cmd: LoginCmd{AgentLifetimeArg: "500ms"}, + errMsg: "must be at least 1 second", + }, + { + // Large enough to wrap int64 nanoseconds into a small positive + // duration if multiplied unbounded. + name: "overflowing raw seconds value is an error", + cmd: LoginCmd{AgentLifetimeArg: "18446744074"}, + errMsg: "out of range", + }, + { + name: "duration exceeding uint32 seconds is an error", + cmd: LoginCmd{AgentLifetimeArg: "1193047h"}, + errMsg: "too large", + }, + { + name: "whitespace-only flag value is an error", + cmd: LoginCmd{AgentLifetimeArg: " "}, + errMsg: "empty duration", }, { name: "invalid config value is an error", diff --git a/docs/config.md b/docs/config.md index c65836b0..3ebc1637 100644 --- a/docs/config.md +++ b/docs/config.md @@ -25,7 +25,7 @@ The client config can be used to configure the following values: - **default_provider** By default this is set to the webchooser, which opens a webpage and allows the user to select the OpenID Provider they want by clicking. However if you wish to always connect to one particular OpenID Provider you can set this to the alias of that OpenID Provider and it will skip the web chooser and automatically just open a browser window to that provider. -- **agent_lifetime** How long ssh-agent retains the certificate that `opkssh login` adds to it, either as a duration (`12h`, `45m`) or in seconds (`28800`). If unset it defaults to `24h`, matching the server's default certificate expiration policy. Note the client cannot know the expiration policy of every server it connects to, so this is a client-side bound to keep expired keys from accumulating in the agent rather than a statement of the certificate's validity. The `--lifetime` flag on `opkssh login` overrides this value. +- **agent_lifetime** How long ssh-agent retains the certificate that `opkssh login` adds to it, either as a duration (`12h`, `45m`) or in seconds (`28800`). If unset it defaults to `24h`, matching the server's default certificate expiration policy. Note the client cannot know the expiration policy of every server it connects to, so this is a client-side bound to keep expired keys from accumulating in the agent rather than a statement of the certificate's validity. The `--lifetime` flag on `opkssh login` overrides this value. On Windows the agent is not currently reachable (the native OpenSSH agent listens on a named pipe rather than a unix socket), so the certificate is not added there. - **providers** This allows you to configure all the OpenID Providers you wish to use. See example below. - **send_access_token** Is a boolean value scoped to a particular provider. It determines if opkssh should put the user's access token into the SSH public key (SSH Certificate). This is useful for allowing the opkssh verifier to read claims not available in the ID Token that can only be read from the OpenID Provider's [userinfo endpoint](https://openid.net/specs/openid-connect-core-1_0.html#UserInfo). The opkssh verifier on the SSH server will use the access token to make a call to the OpenID Provider's userinfo endpoint. Configuration option false by default as SSH will send SSH Public Keys to any host you are attempting to SSH into. Before setting this to true carefully consider the security implications of including the access token in the SSH Public key. diff --git a/test/integration/opkssh_test.go b/test/integration/opkssh_test.go index 8dae162d..2ee5ea49 100644 --- a/test/integration/opkssh_test.go +++ b/test/integration/opkssh_test.go @@ -39,6 +39,12 @@ func TestMain(m *testing.M) { defer cancel() TestCtx = ctx + // Keep every login flow in the suite away from any real ssh-agent on + // the host: login adds the minted certificate to the agent reachable + // via SSH_AUTH_SOCK, and these tests must never inject test keys into + // a developer's agent. + os.Unsetenv("SSH_AUTH_SOCK") + return m.Run() }()) } From 836046c16244167642749db522b4e091def20a71 Mon Sep 17 00:00:00 2001 From: Jason Bowman Date: Thu, 20 Aug 2026 18:41:39 -0700 Subject: [PATCH 03/13] refactor: dedupe test scaffolding, trim comments, document --lifetime in CLI docs Post-review cleanup, behavior-preserving: - The in-process test-agent scaffolding is extracted to startTestAgent (windows skip, socket lifecycle via t.Cleanup, SSH_AUTH_SOCK wiring in one place). - The SSH_AUTH_SOCK isolation guard moves into the shared Mocks fixture, so every mock login flow is isolated from the developer's real agent by construction instead of by three pasted guards; a test that needs an agent points SSH_AUTH_SOCK at its own after calling Mocks. - TestParseConfigWithAgentLifetime collapses its two verbatim copies into a loop over both accepted value shapes. - Two comment blocks that restated nearby doc comments are trimmed. - docs/cli/opkssh_login.md is regenerated to document the new --lifetime flag (only this file has content changes from the flag addition). --- commands/config/client_config_test.go | 29 +++++++----------- commands/login.go | 14 ++++----- commands/login_test.go | 42 ++++++++++++++------------- docs/cli/opkssh_login.md | 3 +- 4 files changed, 39 insertions(+), 49 deletions(-) diff --git a/commands/config/client_config_test.go b/commands/config/client_config_test.go index cf2ca396..ce713ead 100644 --- a/commands/config/client_config_test.go +++ b/commands/config/client_config_test.go @@ -78,28 +78,19 @@ providers: } func TestParseConfigWithAgentLifetime(t *testing.T) { - c := `--- -agent_lifetime: 12h -providers: - - alias: google - issuer: https://accounts.google.com - client_id: test-client-id` - - clientConfig, err := NewClientConfig([]byte(c)) - require.NoError(t, err) - require.NotNil(t, clientConfig) - require.Equal(t, "12h", clientConfig.AgentLifetime) - - // Test with numeric seconds - c2 := `--- -agent_lifetime: 28800 + // Both value shapes must parse into the string field: a duration string + // and a bare integer number of seconds. + for _, lifetime := range []string{"12h", "28800"} { + c := `--- +agent_lifetime: ` + lifetime + ` providers: - alias: google issuer: https://accounts.google.com client_id: test-client-id` - clientConfig2, err := NewClientConfig([]byte(c2)) - require.NoError(t, err) - require.NotNil(t, clientConfig2) - require.Equal(t, "28800", clientConfig2.AgentLifetime) + clientConfig, err := NewClientConfig([]byte(c)) + require.NoError(t, err) + require.NotNil(t, clientConfig) + require.Equal(t, lifetime, clientConfig.AgentLifetime) + } } diff --git a/commands/login.go b/commands/login.go index 599e1d67..64006a58 100644 --- a/commands/login.go +++ b/commands/login.go @@ -573,10 +573,7 @@ func (l *LoginCmd) login(ctx context.Context, provider providers.OpenIdProvider, return nil, err } - // Also load the certificate and its private key into a running ssh-agent - // (if one is reachable via SSH_AUTH_SOCK). Best-effort and non-fatal: the - // keys were already written to disk above, so a missing or broken agent - // must not fail the login. + // Best-effort: addCertToAgent warns and returns rather than failing the login. if !l.PrintKeyArg { l.addCertToAgent(certBytes, signer) } @@ -696,11 +693,10 @@ func (l *LoginCmd) out() io.Writer { return os.Stdout } -// defaultAgentLifetime is how long ssh-agent retains the certificate when -// neither --lifetime nor agent_lifetime in the client config is set. It matches -// the opkssh server's default certificate expiration policy of 24 hours from -// the ID token's issuance (see docs/config.md), so by default the agent drops -// the key around the time the server stops accepting it. +// defaultAgentLifetime matches the opkssh server's default certificate +// expiration policy of 24 hours from the ID token's issuance (see +// docs/config.md), so by default the agent drops the key around the time the +// server stops accepting it. const defaultAgentLifetime = 24 * time.Hour // addCertToAgent loads the freshly minted certificate and its private key into diff --git a/commands/login_test.go b/commands/login_test.go index 06e7c31a..8c2c13cf 100644 --- a/commands/login_test.go +++ b/commands/login_test.go @@ -79,6 +79,11 @@ func Mocks(t *testing.T, keyType KeyType, extraClaims ...map[string]any) (*pktok } require.NoError(t, err) + // Keep every mock login flow away from any real ssh-agent the test + // runner has; a test that needs an agent points SSH_AUTH_SOCK at its own + // after calling Mocks. + t.Setenv("SSH_AUTH_SOCK", "") + providerOpts := providers.DefaultMockProviderOpts() op, _, idtTemplate, err := providers.NewMockProvider(providerOpts) require.NoError(t, err) @@ -102,9 +107,6 @@ func Mocks(t *testing.T, keyType KeyType, extraClaims ...map[string]any) (*pktok } func TestLoginCmd(t *testing.T) { - // Keep the login flow away from any real ssh-agent the test runner has. - t.Setenv("SSH_AUTH_SOCK", "") - logDir := "./logs" logPath := filepath.Join(logDir, "opkssh.log") @@ -632,31 +634,37 @@ func (r *recordingAgent) Add(key agent.AddedKey) error { return r.Agent.Add(key) } -func TestAddCertToAgent(t *testing.T) { +// startTestAgent serves an in-process ssh-agent over a unix socket for the +// duration of the test and points SSH_AUTH_SOCK at it. +func startTestAgent(t *testing.T, a agent.Agent) string { + t.Helper() if runtime.GOOS == "windows" { t.Skip("test agent listens on a unix domain socket") } - - pkt, signer, _ := Mocks(t, ECDSA) - certBytes, _, err := createSSHCert(pkt, signer, []string{"test"}) - require.NoError(t, err) - sockPath := filepath.Join(t.TempDir(), "agent.sock") listener, err := net.Listen("unix", sockPath) require.NoError(t, err) - defer listener.Close() - - mockAgent := &recordingAgent{Agent: agent.NewKeyring()} + t.Cleanup(func() { _ = listener.Close() }) go func() { conn, err := listener.Accept() if err != nil { return } - _ = agent.ServeAgent(mockAgent, conn) + _ = agent.ServeAgent(a, conn) }() + t.Setenv("SSH_AUTH_SOCK", sockPath) + return sockPath +} + +func TestAddCertToAgent(t *testing.T) { + pkt, signer, _ := Mocks(t, ECDSA) + certBytes, _, err := createSSHCert(pkt, signer, []string{"test"}) + require.NoError(t, err) + + mockAgent := &recordingAgent{Agent: agent.NewKeyring()} + startTestAgent(t, mockAgent) out := &bytes.Buffer{} - t.Setenv("SSH_AUTH_SOCK", sockPath) l := &LoginCmd{AgentLifetimeArg: "2h", OutWriter: out} l.addCertToAgent(certBytes, signer) require.Contains(t, out.String(), "Certificate added to ssh-agent") @@ -933,9 +941,6 @@ func TestWriteKeysToDestination(t *testing.T) { // (including its error return). The success side of this call site is also // covered by TestLoginCmd; the error case here covers the `return nil, err`. func TestLoginWritesKeysToDestination(t *testing.T) { - // Keep the login flow away from any real ssh-agent the test runner has. - t.Setenv("SSH_AUTH_SOCK", "") - home, err := os.UserHomeDir() require.NoError(t, err) @@ -1052,9 +1057,6 @@ func (c *writeCountingFs) count() int { // through the shared helper. The token is given a short expiry so the first // refresh fires immediately, and a bounded context guarantees the loop exits. func TestLoginWithRefreshWritesKeysToDestination(t *testing.T) { - // Keep the login flow away from any real ssh-agent the test runner has. - t.Setenv("SSH_AUTH_SOCK", "") - // Short expiry: LoginWithRefresh waits until ~1 minute before expiry, so a // near-term exp makes the first refresh fire right away. shortExp := map[string]any{ diff --git a/docs/cli/opkssh_login.md b/docs/cli/opkssh_login.md index bbbe1f74..cd4a8c89 100644 --- a/docs/cli/opkssh_login.md +++ b/docs/cli/opkssh_login.md @@ -36,6 +36,7 @@ opkssh login [alias] [flags] -h, --help help for login --inspect-cert Print a human-readable inspection of the generated SSH certificate (public information only) -t, --key-type Key Type Type of key to generate (default ecdsa) + --lifetime string How long ssh-agent retains the certificate when it is added at login, as a duration (e.g. 12h, 45m) or in seconds (e.g. 28800). Overrides agent_lifetime in the client config. Defaults to 24h. --log-dir string Directory to write output logs --principals strings Comma separated list of principals to include in the generated SSH certificate. If not specified it will work for any principal. Do not use unless you know what you are doing. --print-id-token Set this flag to print out the contents of the id_token. Useful for inspecting claims @@ -51,4 +52,4 @@ opkssh login [alias] [flags] * [opkssh](opkssh.md) - SSH with OpenPubkey -###### Auto generated by spf13/cobra on 23-Jun-2026 +###### Auto generated by spf13/cobra on 20-Aug-2026 From a6685cf08a888cf61afc9d92b6008a16fb8d48c9 Mon Sep 17 00:00:00 2001 From: Jason Bowman Date: Thu, 20 Aug 2026 18:55:34 -0700 Subject: [PATCH 04/13] docs: condense comments to their load-bearing constraints --- commands/login.go | 42 +++++++++++++-------------------- commands/login_test.go | 10 +++----- test/integration/opkssh_test.go | 6 ++--- 3 files changed, 22 insertions(+), 36 deletions(-) diff --git a/commands/login.go b/commands/login.go index 64006a58..5e5939ee 100644 --- a/commands/login.go +++ b/commands/login.go @@ -693,18 +693,15 @@ func (l *LoginCmd) out() io.Writer { return os.Stdout } -// defaultAgentLifetime matches the opkssh server's default certificate -// expiration policy of 24 hours from the ID token's issuance (see -// docs/config.md), so by default the agent drops the key around the time the -// server stops accepting it. +// defaultAgentLifetime matches the server's default expiration policy (24h +// from the ID token's iat, see docs/config.md): the agent drops the key about +// when the server stops accepting it. const defaultAgentLifetime = 24 * time.Hour -// addCertToAgent loads the freshly minted certificate and its private key into -// the ssh-agent reachable via SSH_AUTH_SOCK. The key is always added with a -// lifetime: ssh-agent has no replace operation, so without one every login -// would accumulate another key in the agent forever. Best-effort: any problem -// is reported and swallowed so login still succeeds (the keys were already -// written to disk before this is called). +// addCertToAgent loads the certificate and its private key into the ssh-agent +// reachable via SSH_AUTH_SOCK, always with a lifetime — ssh-agent has no +// replace operation, so lifetime-less keys would accumulate forever. +// Best-effort: problems are warnings, never login failures. func (l *LoginCmd) addCertToAgent(certBytes []byte, signer crypto.Signer) { sock := os.Getenv("SSH_AUTH_SOCK") if sock == "" { @@ -735,9 +732,8 @@ func (l *LoginCmd) addCertToAgent(certBytes []byte, signer crypto.Signer) { } defer conn.Close() - // Deadline on the whole exchange: a dead socket (e.g. a forwarded agent - // whose upstream connection is gone) accepts the dial but never responds, - // which would otherwise hang the login after authentication succeeded. + // A dead socket (e.g. an orphaned forwarding socket) accepts the dial but + // never responds; without a deadline that hangs login after auth succeeded. if err := conn.SetDeadline(time.Now().Add(5 * time.Second)); err != nil { fmt.Fprintf(l.out(), "warning: could not set ssh-agent I/O deadline: %v\n", err) return @@ -755,12 +751,10 @@ func (l *LoginCmd) addCertToAgent(certBytes []byte, signer crypto.Signer) { fmt.Fprintf(l.out(), "Certificate added to ssh-agent (lifetime %s)\n", time.Duration(lifetimeSecs)*time.Second) } -// resolveAgentLifetimeSecs returns how long, in seconds, ssh-agent should -// retain the certificate: the --lifetime flag if set, otherwise agent_lifetime -// from the client config, otherwise defaultAgentLifetime. The client cannot -// know the server's real expiration policy (the server computes it from the ID -// token's iat claim), so this is a client-side bound that keeps keys from -// accumulating in the agent, not a statement of the certificate's validity. +// resolveAgentLifetimeSecs picks the agent retention: --lifetime flag, else +// agent_lifetime from the client config, else defaultAgentLifetime. It is a +// client-side accumulation bound, not a validity statement — the client +// cannot know the server's expiration policy (computed server-side from iat). func (l *LoginCmd) resolveAgentLifetimeSecs() (uint32, error) { var source, value string switch { @@ -776,9 +770,8 @@ func (l *LoginCmd) resolveAgentLifetimeSecs() (uint32, error) { if err != nil { return 0, fmt.Errorf("invalid %s value %q: %w", source, value, err) } - // Below one second the conversion would truncate to LifetimeSecs 0, which - // the agent protocol treats as "no lifetime" — an immortal key, the exact - // state the lifetime exists to prevent. + // Sub-second durations truncate to LifetimeSecs 0 = "no lifetime" in the + // agent protocol — an immortal key, the exact state this exists to prevent. if d < time.Second { return 0, fmt.Errorf("invalid %s value %q: must be at least 1 second", source, value) } @@ -799,9 +792,8 @@ func parseDurationOrSeconds(s string) (time.Duration, error) { return d, nil } if sec, err := strconv.ParseInt(s, 10, 64); err == nil { - // Bound before multiplying: time.Duration(sec) * time.Second overflows - // int64 for large values and can wrap around into a small positive - // duration. + // Bound first: the multiplication overflows int64 for large values + // and can wrap into a small positive duration. if sec < 0 || sec > math.MaxUint32 { return 0, fmt.Errorf("seconds value %d out of range", sec) } diff --git a/commands/login_test.go b/commands/login_test.go index 8c2c13cf..7a908449 100644 --- a/commands/login_test.go +++ b/commands/login_test.go @@ -79,9 +79,8 @@ func Mocks(t *testing.T, keyType KeyType, extraClaims ...map[string]any) (*pktok } require.NoError(t, err) - // Keep every mock login flow away from any real ssh-agent the test - // runner has; a test that needs an agent points SSH_AUTH_SOCK at its own - // after calling Mocks. + // Isolate every mock login from the test runner's real ssh-agent; a test + // needing an agent points SSH_AUTH_SOCK at its own after calling Mocks. t.Setenv("SSH_AUTH_SOCK", "") providerOpts := providers.DefaultMockProviderOpts() @@ -576,15 +575,12 @@ func TestResolveAgentLifetimeSecs(t *testing.T) { errMsg: "must be at least 1 second", }, { - // Sub-second values would truncate to LifetimeSecs 0, which the - // agent protocol treats as no lifetime at all. name: "sub-second flag value is an error", cmd: LoginCmd{AgentLifetimeArg: "500ms"}, errMsg: "must be at least 1 second", }, { - // Large enough to wrap int64 nanoseconds into a small positive - // duration if multiplied unbounded. + // Wraps int64 nanoseconds if multiplied unbounded. name: "overflowing raw seconds value is an error", cmd: LoginCmd{AgentLifetimeArg: "18446744074"}, errMsg: "out of range", diff --git a/test/integration/opkssh_test.go b/test/integration/opkssh_test.go index 2ee5ea49..a69ace74 100644 --- a/test/integration/opkssh_test.go +++ b/test/integration/opkssh_test.go @@ -39,10 +39,8 @@ func TestMain(m *testing.M) { defer cancel() TestCtx = ctx - // Keep every login flow in the suite away from any real ssh-agent on - // the host: login adds the minted certificate to the agent reachable - // via SSH_AUTH_SOCK, and these tests must never inject test keys into - // a developer's agent. + // login adds the minted certificate to the agent at SSH_AUTH_SOCK; + // never inject test keys into a developer's real agent. os.Unsetenv("SSH_AUTH_SOCK") return m.Run() From 3e337953b362b2b944dbbb39e4fea84bde4c075f Mon Sep 17 00:00:00 2001 From: Jason Bowman Date: Thu, 20 Aug 2026 18:58:08 -0700 Subject: [PATCH 05/13] docs: ascii-only comments --- commands/login.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/commands/login.go b/commands/login.go index 5e5939ee..31fdbf10 100644 --- a/commands/login.go +++ b/commands/login.go @@ -699,7 +699,7 @@ func (l *LoginCmd) out() io.Writer { const defaultAgentLifetime = 24 * time.Hour // addCertToAgent loads the certificate and its private key into the ssh-agent -// reachable via SSH_AUTH_SOCK, always with a lifetime — ssh-agent has no +// reachable via SSH_AUTH_SOCK, always with a lifetime: ssh-agent has no // replace operation, so lifetime-less keys would accumulate forever. // Best-effort: problems are warnings, never login failures. func (l *LoginCmd) addCertToAgent(certBytes []byte, signer crypto.Signer) { @@ -753,7 +753,7 @@ func (l *LoginCmd) addCertToAgent(certBytes []byte, signer crypto.Signer) { // resolveAgentLifetimeSecs picks the agent retention: --lifetime flag, else // agent_lifetime from the client config, else defaultAgentLifetime. It is a -// client-side accumulation bound, not a validity statement — the client +// client-side accumulation bound, not a validity statement; the client // cannot know the server's expiration policy (computed server-side from iat). func (l *LoginCmd) resolveAgentLifetimeSecs() (uint32, error) { var source, value string @@ -771,7 +771,7 @@ func (l *LoginCmd) resolveAgentLifetimeSecs() (uint32, error) { return 0, fmt.Errorf("invalid %s value %q: %w", source, value, err) } // Sub-second durations truncate to LifetimeSecs 0 = "no lifetime" in the - // agent protocol — an immortal key, the exact state this exists to prevent. + // agent protocol; an immortal key, the exact state this exists to prevent. if d < time.Second { return 0, fmt.Errorf("invalid %s value %q: must be at least 1 second", source, value) } From 52a264023319981ef8d4f178c2e528d98c9e35f6 Mon Sep 17 00:00:00 2001 From: Jason Bowman Date: Thu, 20 Aug 2026 19:06:14 -0700 Subject: [PATCH 06/13] docs: clarify comments; every sentence names its subject --- commands/login.go | 40 +++++++++++++++++++-------------- commands/login_test.go | 8 ++++--- test/integration/opkssh_test.go | 5 +++-- 3 files changed, 31 insertions(+), 22 deletions(-) diff --git a/commands/login.go b/commands/login.go index 31fdbf10..778f2ab8 100644 --- a/commands/login.go +++ b/commands/login.go @@ -205,9 +205,9 @@ func (l *LoginCmd) Run(ctx context.Context) error { l.checkSSHConfigured() } - // Validate the requested ssh-agent lifetime up front so that a typo in + // Validate the requested ssh-agent lifetime up front, so that a typo in // --lifetime (or agent_lifetime in the client config) fails before the - // browser dance rather than being discovered after authentication. + // browser-based OIDC flow starts rather than after authentication. if _, err := l.resolveAgentLifetimeSecs(); err != nil { return err } @@ -693,15 +693,16 @@ func (l *LoginCmd) out() io.Writer { return os.Stdout } -// defaultAgentLifetime matches the server's default expiration policy (24h -// from the ID token's iat, see docs/config.md): the agent drops the key about -// when the server stops accepting it. +// defaultAgentLifetime matches the opkssh server's default certificate +// expiration policy (24h from the ID token's iat claim, see docs/config.md), +// so the agent drops the key around the time servers stop accepting it. const defaultAgentLifetime = 24 * time.Hour // addCertToAgent loads the certificate and its private key into the ssh-agent // reachable via SSH_AUTH_SOCK, always with a lifetime: ssh-agent has no -// replace operation, so lifetime-less keys would accumulate forever. -// Best-effort: problems are warnings, never login failures. +// replace operation, so a key without a lifetime would sit in the agent +// forever and every login would add another. Best-effort: every failure is a +// printed warning, never a login failure. func (l *LoginCmd) addCertToAgent(certBytes []byte, signer crypto.Signer) { sock := os.Getenv("SSH_AUTH_SOCK") if sock == "" { @@ -732,8 +733,9 @@ func (l *LoginCmd) addCertToAgent(certBytes []byte, signer crypto.Signer) { } defer conn.Close() - // A dead socket (e.g. an orphaned forwarding socket) accepts the dial but - // never responds; without a deadline that hangs login after auth succeeded. + // A dead socket (e.g. one left behind by agent forwarding) accepts the + // dial but never responds; without a deadline the exchange would hang + // the login after authentication has already succeeded. if err := conn.SetDeadline(time.Now().Add(5 * time.Second)); err != nil { fmt.Fprintf(l.out(), "warning: could not set ssh-agent I/O deadline: %v\n", err) return @@ -751,10 +753,12 @@ func (l *LoginCmd) addCertToAgent(certBytes []byte, signer crypto.Signer) { fmt.Fprintf(l.out(), "Certificate added to ssh-agent (lifetime %s)\n", time.Duration(lifetimeSecs)*time.Second) } -// resolveAgentLifetimeSecs picks the agent retention: --lifetime flag, else -// agent_lifetime from the client config, else defaultAgentLifetime. It is a -// client-side accumulation bound, not a validity statement; the client -// cannot know the server's expiration policy (computed server-side from iat). +// resolveAgentLifetimeSecs picks how long ssh-agent retains the key: the +// --lifetime flag, else agent_lifetime from the client config, else +// defaultAgentLifetime. The result only bounds agent retention; it says +// nothing about how long the certificate is valid, because the client cannot +// know a server's expiration policy (servers compute it from the ID token's +// iat claim). func (l *LoginCmd) resolveAgentLifetimeSecs() (uint32, error) { var source, value string switch { @@ -770,8 +774,9 @@ func (l *LoginCmd) resolveAgentLifetimeSecs() (uint32, error) { if err != nil { return 0, fmt.Errorf("invalid %s value %q: %w", source, value, err) } - // Sub-second durations truncate to LifetimeSecs 0 = "no lifetime" in the - // agent protocol; an immortal key, the exact state this exists to prevent. + // A sub-second duration truncates to LifetimeSecs 0, which the agent + // protocol treats as no lifetime at all: the key would live forever, the + // exact state the lifetime exists to prevent. if d < time.Second { return 0, fmt.Errorf("invalid %s value %q: must be at least 1 second", source, value) } @@ -792,8 +797,9 @@ func parseDurationOrSeconds(s string) (time.Duration, error) { return d, nil } if sec, err := strconv.ParseInt(s, 10, 64); err == nil { - // Bound first: the multiplication overflows int64 for large values - // and can wrap into a small positive duration. + // Check the bound before multiplying: time.Duration(sec)*time.Second + // overflows int64 for large values and can wrap into a small + // positive duration. if sec < 0 || sec > math.MaxUint32 { return 0, fmt.Errorf("seconds value %d out of range", sec) } diff --git a/commands/login_test.go b/commands/login_test.go index 7a908449..7ecc7727 100644 --- a/commands/login_test.go +++ b/commands/login_test.go @@ -79,8 +79,9 @@ func Mocks(t *testing.T, keyType KeyType, extraClaims ...map[string]any) (*pktok } require.NoError(t, err) - // Isolate every mock login from the test runner's real ssh-agent; a test - // needing an agent points SSH_AUTH_SOCK at its own after calling Mocks. + // Isolate every mock login from the test runner's real ssh-agent. A test + // that needs an agent points SSH_AUTH_SOCK at its own test agent after + // calling Mocks. t.Setenv("SSH_AUTH_SOCK", "") providerOpts := providers.DefaultMockProviderOpts() @@ -580,7 +581,8 @@ func TestResolveAgentLifetimeSecs(t *testing.T) { errMsg: "must be at least 1 second", }, { - // Wraps int64 nanoseconds if multiplied unbounded. + // This value wraps int64 nanoseconds if multiplied without a + // bound check. name: "overflowing raw seconds value is an error", cmd: LoginCmd{AgentLifetimeArg: "18446744074"}, errMsg: "out of range", diff --git a/test/integration/opkssh_test.go b/test/integration/opkssh_test.go index a69ace74..85978e1c 100644 --- a/test/integration/opkssh_test.go +++ b/test/integration/opkssh_test.go @@ -39,8 +39,9 @@ func TestMain(m *testing.M) { defer cancel() TestCtx = ctx - // login adds the minted certificate to the agent at SSH_AUTH_SOCK; - // never inject test keys into a developer's real agent. + // login adds the minted certificate to the ssh-agent at + // SSH_AUTH_SOCK; unset it so these tests never inject keys into a + // developer's real agent. os.Unsetenv("SSH_AUTH_SOCK") return m.Run() From 7a1f2bc4d3d74ce8bfa246fa66f5d86f46c9ff37 Mon Sep 17 00:00:00 2001 From: Jason Bowman Date: Thu, 20 Aug 2026 17:44:56 -0700 Subject: [PATCH 07/13] feat: XDG config paths and non-clobbering multi-identity key files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two storage-layout changes, predicates for upcoming refresh-daemon work: Client config resolution now follows the XDG Base Directory spec: the first existing of $XDG_CONFIG_HOME/opk/config.yml (when set and absolute; ~/.config/opk/config.yml otherwise, %AppData%\opk\config.yml on Windows) and the legacy ~/.opk/config.yml. An existing legacy config keeps working untouched and always wins when it is the only one; fresh --create-config writes land at the XDG location with 0700/0600 (the config can carry client_secret values). Login logs which config file is in use at -v. The old --config-path help text claiming %APPDATA%\.opk on Windows was wrong (the code always used ~/.opk everywhere) and is fixed. Key files no longer clobber across identities: the default ~/.ssh slot rule compares the (iss, aud, sub) identity parsed from the existing certificate's embedded PK token, failing closed, instead of trusting the "openpubkey" comment alone — a certificate whose PK token no longer parses but whose comment is exactly "openpubkey" remains reusable, so every legacy file behaves exactly as before. When all default slots belong to other identities, keys are written to the opkssh identity directory (~/.ssh/opkssh, created on demand with its IdentityFile fragment — inert without the --configure Include) named - or, when a different account at the same provider collides, suffixed with an 8-hex sha256(iss|aud|sub) tag; a foreign occupant at the tagged path is an error, never an overwrite. A warning naming the written path fires only when the fallback was used and no ssh-agent ended up holding the key. Intentional behavior changes, called out explicitly: a login whose default slots are all taken now succeeds into the identity directory instead of failing with "no default ssh key file free"; a parseable certificate belonging to a different identity is never overwritten (previously any opkssh cert could clobber any other); a missing opkssh directory or config fragment is created on demand rather than erroring. Single-identity users keep byte-identical behavior throughout. --- README.md | 7 +- commands/config/client_config.go | 70 ++++- commands/config/client_config_test.go | 111 ++++++++ commands/config/config_path_unix.go | 38 +++ commands/config/config_path_windows.go | 28 ++ commands/login.go | 275 ++++++++++++++++---- commands/login_test.go | 324 +++++++++++++++++++++++- docs/cli/opkssh.md | 2 +- docs/cli/opkssh_add.md | 2 +- docs/cli/opkssh_audit.md | 2 +- docs/cli/opkssh_client.md | 2 +- docs/cli/opkssh_client_provider.md | 2 +- docs/cli/opkssh_client_provider_list.md | 4 +- docs/cli/opkssh_inspect.md | 2 +- docs/cli/opkssh_login.md | 4 +- docs/cli/opkssh_logout.md | 2 +- docs/cli/opkssh_permissions.md | 2 +- docs/cli/opkssh_permissions_check.md | 2 +- docs/cli/opkssh_permissions_fix.md | 2 +- docs/cli/opkssh_permissions_install.md | 2 +- docs/cli/opkssh_readhome.md | 2 +- docs/cli/opkssh_verify.md | 2 +- docs/config.md | 18 +- docs/providers/azure.md | 8 +- docs/providers/cognito.md | 4 +- docs/providers/keycloak.md | 4 +- main.go | 6 +- 27 files changed, 833 insertions(+), 94 deletions(-) create mode 100644 commands/config/config_path_unix.go create mode 100644 commands/config/config_path_windows.go diff --git a/README.md b/README.md index f512cefc..f6ff6475 100644 --- a/README.md +++ b/README.md @@ -107,6 +107,7 @@ opkssh login This opens a browser window to select which OpenID Provider you want to authenticate against. After successfully authenticating opkssh generates an SSH public key in `~/.ssh/id_ecdsa` which contains your PK Token. +If the default key files already hold a different identity — for instance when you log in with a second account or provider — the new keys are written to the opkssh identity directory (`~/.ssh/opkssh/`) instead of overwriting them. By default this ssh key expires after 24 hours and you must run `opkssh login` to generate a new ssh key. Since your PK Token has been saved as an SSH key you can SSH as normal: @@ -441,9 +442,9 @@ This alias to provider mapping be can configured using the OPKSSH_PROVIDERS envi ### Client Config File -Rather than type in the provider each time, you can create a client config file by running `opkssh login --create-config` at -`C:\Users\{USER}\.opk\config.yml` on windows and `~/.opk/config.yml` on linux. -You can then edit this config file to add your provider. +Rather than type in the provider each time, you can create a client config file by running `opkssh login --create-config`. +The config is created at `~/.config/opk/config.yml` on linux/macOS (or `$XDG_CONFIG_HOME/opk/config.yml` if set) and `%AppData%\opk\config.yml` on windows; an existing legacy `~/.opk/config.yml` keeps working and takes effect instead. +You can then edit this config file to add your provider. Run `opkssh login -v` to see which config file is in use.
config.yml diff --git a/commands/config/client_config.go b/commands/config/client_config.go index b4333ce7..866d8805 100644 --- a/commands/config/client_config.go +++ b/commands/config/client_config.go @@ -60,21 +60,72 @@ func (c *ClientConfig) GetByIssuer(issuer string) (*ProviderConfig, bool) { return nil, false } -func ResolveClientConfigPath(configPath *string) error { - if *configPath == "" { - dir, dirErr := os.UserHomeDir() - if dirErr != nil { - return fmt.Errorf("failed to get user config dir: %w", dirErr) +// ClientConfigCandidatePaths returns the client config locations in +// resolution order: +// +// 1. /opk/config.yml, where is $XDG_CONFIG_HOME when +// set and absolute (the XDG Base Directory spec requires relative values +// to be ignored), otherwise the platform default (~/.config on Unix-like +// systems, %AppData% on Windows). Replacement semantics per the spec: a +// set variable replaces the platform default, it does not stack with it. +// 2. The legacy ~/.opk/config.yml. +func ClientConfigCandidatePaths() ([]string, error) { + var configDir string + var platformDirErr error + if xdgDir := os.Getenv("XDG_CONFIG_HOME"); xdgDir != "" && filepath.IsAbs(xdgDir) { + configDir = xdgDir + } else if platformDir, err := userConfigDir(); err == nil { + configDir = platformDir + } else { + platformDirErr = err + } + + var candidates []string + if configDir != "" { + candidates = append(candidates, filepath.Join(configDir, "opk", "config.yml")) + } + if homeDir, err := os.UserHomeDir(); err == nil { + candidates = append(candidates, filepath.Join(homeDir, ".opk", "config.yml")) + } else { + // Never drop the legacy candidate silently: a user with only a + // legacy config would otherwise get an unexplained fresh-config + // resolution. + log.Printf("warning: could not determine home directory, ignoring legacy ~/.opk/config.yml: %v", err) + } + if len(candidates) == 0 { + return nil, fmt.Errorf("failed to determine the user config directory: %w", platformDirErr) + } + return candidates, nil +} + +// ResolveClientConfigPath resolves the client config path. An explicitly +// provided path is used as-is. Otherwise the first existing candidate wins — +// so a legacy ~/.opk/config.yml keeps working untouched — and when no config +// exists anywhere the path falls to the first candidate, the XDG-preferred +// location, which is where a new config is then created. +func ResolveClientConfigPath(fs afero.Fs, configPath *string) error { + if *configPath != "" { + return nil + } + candidates, err := ClientConfigCandidatePaths() + if err != nil { + return err + } + afs := &afero.Afero{Fs: fs} + for _, candidate := range candidates { + if exists, err := afs.Exists(candidate); err == nil && exists { + *configPath = candidate + return nil } - *configPath = filepath.Join(dir, ".opk", "config.yml") } + *configPath = candidates[0] return nil } // GetClientConfigFromFile retrieves the client config from the configuration file at configPath. // If configPath is not specified then the default configuration path is uses ~/.opk/config.yml func GetClientConfigFromFile(configPath string, Fs afero.Fs) (*ClientConfig, error) { - if err := ResolveClientConfigPath(&configPath); err != nil { + if err := ResolveClientConfigPath(Fs, &configPath); err != nil { return nil, err } @@ -94,10 +145,11 @@ func GetClientConfigFromFile(configPath string, Fs afero.Fs) (*ClientConfig, err func CreateDefaultClientConfig(configPath string, Fs afero.Fs) error { afs := &afero.Afero{Fs: Fs} - if err := afs.MkdirAll(filepath.Dir(configPath), 0o755); err != nil { + // 0700/0600: the client config can carry provider client_secret values. + if err := afs.MkdirAll(filepath.Dir(configPath), 0o700); err != nil { return fmt.Errorf("failed to create config directory: %w", err) } - if err := afs.WriteFile(configPath, DefaultClientConfig, 0o644); err != nil { + if err := afs.WriteFile(configPath, DefaultClientConfig, 0o600); err != nil { return fmt.Errorf("failed to write default config file: %w", err) } log.Printf("created client config file at %s", configPath) diff --git a/commands/config/client_config_test.go b/commands/config/client_config_test.go index ce713ead..395730fe 100644 --- a/commands/config/client_config_test.go +++ b/commands/config/client_config_test.go @@ -17,8 +17,11 @@ package config import ( + "os" + "runtime" "testing" + "github.com/spf13/afero" "github.com/stretchr/testify/require" ) @@ -94,3 +97,111 @@ providers: require.Equal(t, lifetime, clientConfig.AgentLifetime) } } + +func TestResolveClientConfigPath(t *testing.T) { + const home = "/home/testuser" + xdgPath := "/xdg-config/opk/config.yml" + platformPath := home + "/.config/opk/config.yml" + legacyPath := home + "/.opk/config.yml" + + tests := []struct { + name string + xdgEnv string + files []string + explicit string + expected string + }{ + { + name: "explicit path is used as-is", + explicit: "/tmp/custom.yml", + files: []string{legacyPath}, + expected: "/tmp/custom.yml", + }, + { + name: "XDG set and file exists there", + xdgEnv: "/xdg-config", + files: []string{xdgPath, legacyPath}, + expected: xdgPath, + }, + { + name: "XDG replaces the platform dir, it does not stack", + xdgEnv: "/xdg-config", + files: []string{platformPath}, + expected: xdgPath, // ~/.config is never consulted; no legacy -> chain head + }, + { + name: "relative XDG value is ignored per the spec", + xdgEnv: "relative/dir", + files: []string{platformPath}, + expected: platformPath, + }, + { + name: "platform dir file wins over legacy", + files: []string{platformPath, legacyPath}, + expected: platformPath, + }, + { + name: "legacy config keeps working when it is the only one", + files: []string{legacyPath}, + expected: legacyPath, + }, + { + // The common upgrade scenario: XDG in the environment, but the + // user's only config is the legacy one. + name: "XDG set but only legacy exists: legacy wins", + xdgEnv: "/xdg-config", + files: []string{legacyPath}, + expected: legacyPath, + }, + { + name: "no config anywhere resolves to the chain head for creation", + files: nil, + expected: platformPath, + }, + { + name: "no config anywhere with XDG set resolves to the XDG head", + xdgEnv: "/xdg-config", + files: nil, + expected: xdgPath, + }, + } + + if runtime.GOOS == "windows" { + // Same guard as TestConfigureSSHHomeDirError: the home directory is + // not resolved via HOME on Windows, and unix-style absolute paths + // are not absolute there, so the table's paths cannot apply. + t.Skip("home directory is not resolved via HOME on Windows") + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Setenv("HOME", home) + t.Setenv("XDG_CONFIG_HOME", tt.xdgEnv) + + fs := afero.NewMemMapFs() + for _, f := range tt.files { + require.NoError(t, afero.WriteFile(fs, f, []byte("---\n"), 0o600)) + } + + configPath := tt.explicit + require.NoError(t, ResolveClientConfigPath(fs, &configPath)) + require.Equal(t, tt.expected, configPath) + }) + } +} + +func TestCreateDefaultClientConfigPerms(t *testing.T) { + // The client config can carry provider client_secret values, so new + // creates must be 0700 (dir) / 0600 (file). + fs := afero.NewMemMapFs() + configPath := "/home/testuser/.config/opk/config.yml" + require.NoError(t, CreateDefaultClientConfig(configPath, fs)) + + fileInfo, err := fs.Stat(configPath) + require.NoError(t, err) + require.Equal(t, os.FileMode(0o600), fileInfo.Mode().Perm()) + + dirInfo, err := fs.Stat("/home/testuser/.config/opk") + require.NoError(t, err) + require.Equal(t, os.FileMode(0o700), dirInfo.Mode().Perm()) +} diff --git a/commands/config/config_path_unix.go b/commands/config/config_path_unix.go new file mode 100644 index 00000000..3a8ea20c --- /dev/null +++ b/commands/config/config_path_unix.go @@ -0,0 +1,38 @@ +//go:build !windows +// +build !windows + +// Copyright 2026 OpenPubkey +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +// SPDX-License-Identifier: Apache-2.0 + +package config + +import ( + "os" + "path/filepath" +) + +// userConfigDir returns the platform's user config directory used when +// $XDG_CONFIG_HOME is not set. This is deliberately ~/.config on every +// Unix-like system, macOS included: opkssh is a CLI tool, and CLI convention +// (and the XDG default) is ~/.config rather than ~/Library/Application +// Support. +func userConfigDir() (string, error) { + homeDir, err := os.UserHomeDir() + if err != nil { + return "", err + } + return filepath.Join(homeDir, ".config"), nil +} diff --git a/commands/config/config_path_windows.go b/commands/config/config_path_windows.go new file mode 100644 index 00000000..921c7379 --- /dev/null +++ b/commands/config/config_path_windows.go @@ -0,0 +1,28 @@ +//go:build windows +// +build windows + +// Copyright 2026 OpenPubkey +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +// SPDX-License-Identifier: Apache-2.0 + +package config + +import "os" + +// userConfigDir returns the platform's user config directory used when +// $XDG_CONFIG_HOME is not set: %AppData% on Windows. +func userConfigDir() (string, error) { + return os.UserConfigDir() +} diff --git a/commands/login.go b/commands/login.go index 778f2ab8..ec3b1168 100644 --- a/commands/login.go +++ b/commands/login.go @@ -21,7 +21,9 @@ import ( "context" "crypto" "crypto/ecdsa" + "crypto/sha256" "encoding/base64" + "encoding/hex" "encoding/json" "encoding/pem" "errors" @@ -109,6 +111,10 @@ type LoginCmd struct { overrideProvider *providers.OpenIdProvider // Used in tests to override the provider to inject a mock provider // State Config *config.ClientConfig + // fallbackKeyPath is set when the default key slots were all taken by + // other identities and the keys were written to the opkssh identity + // directory instead; used to warn when that key is not reachable. + fallbackKeyPath string // Outputs pkt *pktoken.PKToken @@ -169,7 +175,7 @@ func (l *LoginCmd) Run(ctx context.Context) error { // If the Config has been set in the struct don't replace it. This is useful for testing if l.Config == nil { - if err := config.ResolveClientConfigPath(&l.ConfigPathArg); err != nil { + if err := config.ResolveClientConfigPath(l.Fs, &l.ConfigPathArg); err != nil { return err } if _, err := l.Fs.Stat(l.ConfigPathArg); err == nil { @@ -181,6 +187,13 @@ func (l *LoginCmd) Run(ctx context.Context) error { return err } else { l.Config = client_config + // Multiple config locations are possible (see + // config.ClientConfigCandidatePaths); naming the winner + // defuses silent shadowing, e.g. a hand-created legacy file + // losing to an existing XDG-location config. + if l.Verbosity >= 1 { + log.Printf("using client config at %s", l.ConfigPathArg) + } } } else { if l.CreateConfigArg { @@ -192,6 +205,9 @@ func (l *LoginCmd) Run(ctx context.Context) error { if err != nil { return fmt.Errorf("failed to parse default config file: %w", err) } + if l.Verbosity >= 1 { + log.Printf("no client config file found; using built-in defaults (a new config would be created at %s)", l.ConfigPathArg) + } } } @@ -575,7 +591,14 @@ func (l *LoginCmd) login(ctx context.Context, provider providers.OpenIdProvider, // Best-effort: addCertToAgent warns and returns rather than failing the login. if !l.PrintKeyArg { - l.addCertToAgent(certBytes, signer) + addedToAgent := l.addCertToAgent(certBytes, signer) + // The remediation warning is decided after the agent attempt: keys + // written to the fallback location are not tried by ssh on their + // own, so the user must hear about it exactly when no agent ended up + // holding the key either. + if l.fallbackKeyPath != "" && !addedToAgent { + fmt.Fprintf(l.out(), "warning: ssh will not try %s automatically and no ssh-agent holds it; add an IdentityFile entry for it, use -i, or run `opkssh login --configure`\n", l.fallbackKeyPath) + } } if printIdToken { @@ -699,37 +722,38 @@ func (l *LoginCmd) out() io.Writer { const defaultAgentLifetime = 24 * time.Hour // addCertToAgent loads the certificate and its private key into the ssh-agent -// reachable via SSH_AUTH_SOCK, always with a lifetime: ssh-agent has no -// replace operation, so a key without a lifetime would sit in the agent -// forever and every login would add another. Best-effort: every failure is a -// printed warning, never a login failure. -func (l *LoginCmd) addCertToAgent(certBytes []byte, signer crypto.Signer) { +// reachable via SSH_AUTH_SOCK, reporting whether the key landed in an agent. +// Always with a lifetime: ssh-agent has no replace operation, so a key +// without a lifetime would sit in the agent forever and every login would +// add another. Best-effort: every failure is a printed warning, never a +// login failure. +func (l *LoginCmd) addCertToAgent(certBytes []byte, signer crypto.Signer) bool { sock := os.Getenv("SSH_AUTH_SOCK") if sock == "" { - return + return false } lifetimeSecs, err := l.resolveAgentLifetimeSecs() if err != nil { fmt.Fprintf(l.out(), "warning: not adding certificate to ssh-agent: %v\n", err) - return + return false } pubkey, _, _, _, err := ssh.ParseAuthorizedKey(certBytes) if err != nil { fmt.Fprintf(l.out(), "warning: could not parse generated certificate for ssh-agent: %v\n", err) - return + return false } cert, ok := pubkey.(*ssh.Certificate) if !ok { fmt.Fprintln(l.out(), "warning: generated key is not a certificate; not adding to ssh-agent") - return + return false } conn, err := net.Dial("unix", sock) if err != nil { fmt.Fprintf(l.out(), "warning: could not connect to ssh-agent (%s): %v\n", sock, err) - return + return false } defer conn.Close() @@ -738,7 +762,7 @@ func (l *LoginCmd) addCertToAgent(certBytes []byte, signer crypto.Signer) { // the login after authentication has already succeeded. if err := conn.SetDeadline(time.Now().Add(5 * time.Second)); err != nil { fmt.Fprintf(l.out(), "warning: could not set ssh-agent I/O deadline: %v\n", err) - return + return false } if err := agent.NewClient(conn).Add(agent.AddedKey{ @@ -748,9 +772,10 @@ func (l *LoginCmd) addCertToAgent(certBytes []byte, signer crypto.Signer) { LifetimeSecs: lifetimeSecs, }); err != nil { fmt.Fprintf(l.out(), "warning: failed to add certificate to ssh-agent: %v\n", err) - return + return false } fmt.Fprintf(l.out(), "Certificate added to ssh-agent (lifetime %s)\n", time.Duration(lifetimeSecs)*time.Second) + return true } // resolveAgentLifetimeSecs picks how long ssh-agent retains the key: the @@ -873,7 +898,7 @@ func (l *LoginCmd) writeKeysToDestination(seckeyPath string, seckeySshPem []byte } return nil case l.SSHConfigured: - if err := l.writeKeysToOpkSSHDir(seckeySshPem, certBytes); err != nil { + if _, err := l.writeKeysToOpkSSHDir(seckeySshPem, certBytes); err != nil { return fmt.Errorf("failed to write SSH keys to OPK SSH dir: %w", err) } return nil @@ -886,7 +911,7 @@ func (l *LoginCmd) writeKeysToDestination(seckeyPath string, seckeySshPem []byte } } -func (l *LoginCmd) writeKeysToOpkSSHDir(secKeyPem []byte, certBytes []byte) error { +func (l *LoginCmd) writeKeysToOpkSSHDir(secKeyPem []byte, certBytes []byte) (string, error) { const ( opkSshPath = ".ssh/opkssh" @@ -895,51 +920,108 @@ func (l *LoginCmd) writeKeysToOpkSSHDir(secKeyPem []byte, certBytes []byte) erro userhomeDir, err := os.UserHomeDir() if err != nil { - return err + return "", err } opkSshUserPath := filepath.Join(userhomeDir, opkSshPath) opkSshConfigPath := filepath.Join(opkSshUserPath, configFileName) - sshKeyName := l.makeSSHKeyFileName(l.pkt) + // The directory and its config fragment are created on demand: this + // writer also serves logins whose default key slots are all taken, which + // may happen before --configure has ever run. Writing the IdentityFile + // line is inert until ~/.ssh/config gains the Include directive, but it + // makes a later --configure pick every key up instantly. + if err := l.Fs.MkdirAll(opkSshUserPath, 0o700); err != nil { + return "", err + } - privKeyPath := filepath.Join(opkSshUserPath, sshKeyName) - pubKeyPath := filepath.Join(privKeyPath + "-cert.pub") + afs := &afero.Afero{Fs: l.Fs} + identity, identityOK := pktIdentity(l.pkt) - // get key comment - issuer, err := l.pkt.Issuer() - if err != nil { - issuer = "unknown" + sshKeyName := l.makeSSHKeyFileName(l.pkt) + privKeyPath := filepath.Join(opkSshUserPath, sshKeyName) + pubKeyPath := privKeyPath + "-cert.pub" + + // A different account at the same provider maps to the same + // - file name; it must not be clobbered. The slot is + // reusable only when the existing certificate provably belongs to the + // same identity; otherwise (different identity, unparseable, or a + // private key without its certificate) disambiguate with an identity + // tag — and apply the same rule at the tagged path, erroring rather + // than silently replacing a foreign occupant. + if !l.slotReusable(privKeyPath, pubKeyPath, identity, identityOK) { + // Tagging a nothing would produce a stable but non-identifying file + // name; failing loudly is clearer (unreachable in practice — the PK + // token just completed a login). + if !identityOK { + return "", errors.New("cannot disambiguate the key file name: no identity could be extracted from the PK token") + } + sshKeyName = sshKeyName + "-" + identityTag(identity) + privKeyPath = filepath.Join(opkSshUserPath, sshKeyName) + pubKeyPath = privKeyPath + "-cert.pub" + if !l.slotReusable(privKeyPath, pubKeyPath, identity, identityOK) { + return "", fmt.Errorf("key file %s exists and holds a different identity; remove it or use -i to pick another path", privKeyPath) + } } - audience, err := l.pkt.Audience() - if err != nil { - audience = "unknown" - } + comment := " openpubkey: " + commentField(identity.issuer) + " " + commentField(identity.audience) - comment := " openpubkey: " + issuer + " " + audience + // Write the key files before the config fragment references them, so a + // key-write failure cannot leave a dangling IdentityFile entry. + if err := l.writeKeysComment(privKeyPath, pubKeyPath, secKeyPem, certBytes, comment); err != nil { + return "", err + } // add key to config - afs := &afero.Afero{Fs: l.Fs} configContent, err := afs.ReadFile(opkSshConfigPath) if err != nil { - return fmt.Errorf("failed to read opk ssh config file (%s): %w", opkSshConfigPath, err) + if !os.IsNotExist(err) { + return "", fmt.Errorf("failed to read opk ssh config file (%s): %w", opkSshConfigPath, err) + } + configContent = nil } - if !strings.Contains(string(configContent), privKeyPath) { + // Whole-line match: a tagged path contains its untagged prefix, so a + // substring check would wrongly skip needed entries. + identityLine := "IdentityFile " + privKeyPath + hasLine := false + for _, line := range strings.Split(string(configContent), "\n") { + if strings.TrimSpace(line) == identityLine { + hasLine = true + break + } + } + if !hasLine { configContent = slices.Concat( - []byte("IdentityFile "+privKeyPath+"\n"), + []byte(identityLine+"\n"), configContent, ) } err = afs.WriteFile(opkSshConfigPath, configContent, 0600) if err != nil { - return fmt.Errorf("failed to write opk ssh config file (%s): %w", opkSshConfigPath, err) + return "", fmt.Errorf("failed to write opk ssh config file (%s): %w", opkSshConfigPath, err) } + return privKeyPath, nil +} - // write ssh key files - return l.writeKeysComment(privKeyPath, pubKeyPath, secKeyPem, certBytes, comment) +// slotReusable reports whether a key file slot may be written for the given +// identity: the slot is free, or its certificate provably belongs to the +// same identity. Identity comparison fails closed — an unparseable existing +// certificate or an unknown own identity never authorizes an overwrite. +func (l *LoginCmd) slotReusable(privKeyPath, pubKeyPath string, identity keyIdentity, identityOK bool) bool { + if !l.fileExists(privKeyPath) && !l.fileExists(pubKeyPath) { + return true + } + if !l.fileExists(pubKeyPath) { + return false + } + existing, err := (&afero.Afero{Fs: l.Fs}).ReadFile(pubKeyPath) + if err != nil { + return false + } + existingIdentity, parsed := certFileIdentity(existing) + return parsed && identityOK && existingIdentity == identity } func (l *LoginCmd) writeKeysToSSHDir(seckeySshPem []byte, certBytes []byte) error { @@ -956,16 +1038,18 @@ func (l *LoginCmd) writeKeysToSSHDir(seckeySshPem []byte, certBytes []byte) erro } // For ssh to automatically find the key created by openpubkey when - // connecting, we use one of the default ssh key paths. However, the file - // might contain an existing key. We will overwrite the key if it was - // generated by openpubkey which we check by looking at the associated - // comment. If the comment is equal to "openpubkey", we overwrite the file - // with a new key. + // connecting, we prefer one of the default ssh key paths. A slot is + // reusable when it is free or when it already holds a certificate for + // the SAME identity (a re-login or refresh); a key belonging to a + // different identity — another opkssh account or a foreign key — is + // never overwritten. keyFileNames, ok := DefaultSSHKeyFileNames[l.KeyTypeArg] if !ok { return fmt.Errorf("key type (%s) has no default output file name; use -i ", l.KeyTypeArg.String()) } + identity, identityOK := pktIdentity(l.pkt) + afs := &afero.Afero{Fs: l.Fs} for _, keyFilename := range keyFileNames { seckeyPath := filepath.Join(sshPath, keyFilename) pubkeyPath := seckeyPath + "-cert.pub" @@ -976,26 +1060,117 @@ func (l *LoginCmd) writeKeysToSSHDir(seckeySshPem []byte, certBytes []byte) erro } else if !l.fileExists(pubkeyPath) { continue } else { - // If the ssh key file does exist, check if it was generated by openpubkey, if it was then it is safe to overwrite - afs := &afero.Afero{Fs: l.Fs} sshPubkey, err := afs.ReadFile(pubkeyPath) if err != nil { log.Println("Failed to read:", pubkeyPath) continue } - _, comment, _, _, err := ssh.ParseAuthorizedKey(sshPubkey) + pubkey, comment, _, _, err := ssh.ParseAuthorizedKey(sshPubkey) if err != nil { log.Println("Failed to parse:", pubkeyPath) continue } - - // If the key comment is "openpubkey" then we generated it + if existingIdentity, parsed := certKeyIdentity(pubkey); parsed { + if identityOK && existingIdentity == identity { + return l.writeKeys(seckeyPath, pubkeyPath, seckeySshPem, certBytes) + } + // A parseable certificate for a different identity is + // protected; try the next slot. + continue + } + // Legacy fallback: an opkssh certificate whose embedded PK token + // no longer parses must still be reusable exactly as before this + // change, which keyed reuse on the comment alone. This repo does + // not control the PK token wire format's history, and demoting a + // re-login to a relocated key on a parse failure would silently + // shadow the fresh key with the stale file. if comment == "openpubkey" { return l.writeKeys(seckeyPath, pubkeyPath, seckeySshPem, certBytes) } } } - return fmt.Errorf("no default ssh key file free for openpubkey") + + // Every default slot belongs to someone else: write a per-identity file + // into the opkssh identity directory instead of clobbering. ssh does not + // try non-default file names on its own — the key is reachable through + // ssh-agent (login loads it there when one is available), an + // IdentityFile entry, -i, or the --configure mechanism. + writtenPath, err := l.writeKeysToOpkSSHDir(seckeySshPem, certBytes) + if err != nil { + return err + } + l.fallbackKeyPath = writtenPath + return nil +} + +// keyIdentity is the identity triple a written key file is bound to. +type keyIdentity struct { + issuer string + audience string + subject string +} + +// pktIdentity extracts the identity triple from a PK token. It fails closed: +// ok is false when the token is nil or any component cannot be extracted, +// and a failed identity must never compare equal to anything — equality is +// what authorizes overwriting a key file. +func pktIdentity(pkt *pktoken.PKToken) (keyIdentity, bool) { + if pkt == nil { + return keyIdentity{}, false + } + issuer, errIss := pkt.Issuer() + audience, errAud := pkt.Audience() + subject, errSub := pkt.Subject() + if errIss != nil || errAud != nil || errSub != nil { + return keyIdentity{}, false + } + return keyIdentity{issuer: issuer, audience: audience, subject: subject}, true +} + +// certFileIdentity extracts the identity from the bytes of a certificate +// file written by opkssh. ok is false when the file does not hold an opkssh +// certificate with an extractable identity (foreign key, plain public key, +// unreadable or unparseable PK token). +func certFileIdentity(certBytes []byte) (keyIdentity, bool) { + pubkey, _, _, _, err := ssh.ParseAuthorizedKey(certBytes) + if err != nil { + return keyIdentity{}, false + } + return certKeyIdentity(pubkey) +} + +// certKeyIdentity extracts the identity from an already-parsed public key. +func certKeyIdentity(pubkey ssh.PublicKey) (keyIdentity, bool) { + cert, isCert := pubkey.(*ssh.Certificate) + if !isCert { + return keyIdentity{}, false + } + smuggler := &sshcert.SshCertSmuggler{SshCert: cert} + pkt, err := smuggler.GetPKToken() + if err != nil { + return keyIdentity{}, false + } + return pktIdentity(pkt) +} + +// commentField preserves the historical key-comment format, which printed +// "unknown" for a claim that could not be extracted rather than an empty +// field. +func commentField(s string) string { + if s == "" { + return "unknown" + } + return s +} + +// identityTag returns a short, stable tag for an identity, used to +// disambiguate key file names for different accounts at the same provider. +// The full triple is hashed — not the subject alone — so that two client IDs +// sharing a truncated prefix in the file name cannot alias, and because some +// OPs use email addresses as subjects, which do not belong in file names. +func identityTag(identity keyIdentity) string { + digest := sha256.Sum256([]byte(identity.issuer + "|" + identity.audience + "|" + identity.subject)) + return hex.EncodeToString(digest[:4]) } func (l *LoginCmd) writeKeys(seckeyPath string, pubkeyPath string, seckeySshPem []byte, certBytes []byte) error { @@ -1055,7 +1230,9 @@ func (l *LoginCmd) makeSSHKeyFileName(pkt *pktoken.PKToken) string { } func (l *LoginCmd) fileExists(fPath string) bool { - _, err := l.Fs.Open(fPath) + // Stat, not Open: the previous Open-based check leaked a file descriptor + // per call. + _, err := l.Fs.Stat(fPath) return !errors.Is(err, os.ErrNotExist) } @@ -1069,7 +1246,7 @@ func IdentityString(pkt pktoken.PKToken) (string, error) { claims := idt.GetClaims() if claims.Email == "" { return fmt.Sprintf(`WARNING: Email claim is missing from ID token. Policies based on email will not work. -Check if your client config (~/.opk/config.yml) has the correct scopes configured for this OpenID Provider. +Check if your client config has the correct scopes configured for this OpenID Provider (run with -v to see which config file is in use). Sub, issuer, audience: %s %s %s`, claims.Subject, claims.Issuer, claims.Audience), nil } else { diff --git a/commands/login_test.go b/commands/login_test.go index 7ecc7727..037e87ab 100644 --- a/commands/login_test.go +++ b/commands/login_test.go @@ -870,9 +870,12 @@ func TestWriteKeysToDestination(t *testing.T) { wantErrContains: "failed to write SSH keys to filesystem", }, { + // A missing opkssh dir or config fragment is no longer an error + // (both are created on demand), so the write failure is induced + // with a read-only filesystem instead. name: "opkssh dir write failure is wrapped", sshConfigured: true, - readOnly: false, // SSHConfigured but no config file -> read fails + readOnly: true, wantErrContains: "failed to write SSH keys to OPK SSH dir", }, { @@ -1427,3 +1430,322 @@ func TestDetermineProviderWebChooserWithOnlyCICDProviders(t *testing.T) { require.ErrorContains(t, err, "no browser-based providers configured") require.Nil(t, chooser) } + +func TestIdentityTag(t *testing.T) { + a := keyIdentity{issuer: "https://op.example.com", audience: "client-1", subject: "alice@example.com"} + b := keyIdentity{issuer: "https://op.example.com", audience: "client-1", subject: "bob@example.com"} + c := keyIdentity{issuer: "https://op.example.com", audience: "client-2", subject: "alice@example.com"} + + require.Len(t, identityTag(a), 8) + require.Equal(t, identityTag(a), identityTag(a), "tag must be deterministic") + require.NotEqual(t, identityTag(a), identityTag(b), "different subjects must yield different tags") + require.NotEqual(t, identityTag(a), identityTag(c), "different audiences must yield different tags") + require.NotContains(t, identityTag(a), "alice", "raw subject must not appear in the tag") +} + +// foreignPubkeyLine returns an authorized-key line for a key opkssh did not +// generate: parseable, not a certificate, comment not "openpubkey". +func foreignPubkeyLine(t *testing.T) []byte { + pub, _, err := ed25519.GenerateKey(rand.Reader) + require.NoError(t, err) + sshPub, err := ssh.NewPublicKey(pub) + require.NoError(t, err) + return ssh.MarshalAuthorizedKey(sshPub) +} + +func TestWriteKeysIdentitySlots(t *testing.T) { + homePath, err := os.UserHomeDir() + require.NoError(t, err) + defaultKeyPath := filepath.Join(homePath, ".ssh", "id_ecdsa") + skKeyPath := filepath.Join(homePath, ".ssh", "id_ecdsa_sk") + opkDirPath := filepath.Join(homePath, ".ssh", "opkssh") + + pktA, signerA, _ := Mocks(t, ECDSA) + pktB, signerB, _ := Mocks(t, ECDSA, map[string]any{ + "email": "second.identity@example.com", + "sub": "second-identity-sub", + }) + identityA, okA := pktIdentity(pktA) + identityB, okB := pktIdentity(pktB) + require.True(t, okA) + require.True(t, okB) + require.NotEqual(t, identityA, identityB, "mock identities must differ for this test to mean anything") + + certA, pemA, err := createSSHCert(pktA, signerA, []string{"test"}) + require.NoError(t, err) + certA2, pemA2, err := createSSHCert(pktA, signerA, []string{"test"}) + require.NoError(t, err) + certB, pemB, err := createSSHCert(pktB, signerB, []string{"test"}) + require.NoError(t, err) + certB2, pemB2, err := createSSHCert(pktB, signerB, []string{"test"}) + require.NoError(t, err) + + fs := afero.NewMemMapFs() + afs := &afero.Afero{Fs: fs} + newCmd := func(pkt *pktoken.PKToken) *LoginCmd { + return &LoginCmd{Fs: fs, KeyTypeArg: ECDSA, pkt: pkt, OutWriter: &bytes.Buffer{}} + } + mustRead := func(path string) []byte { + b, err := afs.ReadFile(path) + require.NoError(t, err) + return b + } + + // Occupy the id_ecdsa_sk slot with a foreign key so the second identity + // exercises the fallback rather than cascading into the _sk slot. + require.NoError(t, afs.WriteFile(skKeyPath, []byte("foreign"), 0o600)) + require.NoError(t, afs.WriteFile(skKeyPath+"-cert.pub", foreignPubkeyLine(t), 0o644)) + + // First login writes the default slot, exactly as before this change. + cmdA := newCmd(pktA) + require.NoError(t, cmdA.writeKeysToDestination("", pemA, certA)) + require.Empty(t, cmdA.fallbackKeyPath) + require.Equal(t, pemA, mustRead(defaultKeyPath)) + + // Same identity again: overwritten in place, still no fallback, and the + // opkssh dir is never created for a single identity. + cmdA2 := newCmd(pktA) + require.NoError(t, cmdA2.writeKeysToDestination("", pemA2, certA2)) + require.Empty(t, cmdA2.fallbackKeyPath) + require.Equal(t, pemA2, mustRead(defaultKeyPath)) + dirExists, err := afs.DirExists(opkDirPath) + require.NoError(t, err) + require.False(t, dirExists, "opkssh dir must not appear while a single identity is in play") + + // Different identity: the occupied slots are preserved and the keys land + // in the opkssh identity directory, with a fragment entry. + cmdB := newCmd(pktB) + require.NoError(t, cmdB.writeKeysToDestination("", pemB, certB)) + require.NotEmpty(t, cmdB.fallbackKeyPath) + require.Equal(t, pemA2, mustRead(defaultKeyPath), "first identity's key must be untouched") + require.Equal(t, []byte("foreign"), mustRead(skKeyPath), "foreign key must be untouched") + require.True(t, strings.HasPrefix(cmdB.fallbackKeyPath, opkDirPath+string(filepath.Separator))) + require.Equal(t, pemB, mustRead(cmdB.fallbackKeyPath)) + writtenIdentity, parsed := certFileIdentity(mustRead(cmdB.fallbackKeyPath + "-cert.pub")) + require.True(t, parsed) + require.Equal(t, identityB, writtenIdentity) + fragmentLines := strings.Split(string(mustRead(filepath.Join(opkDirPath, "config"))), "\n") + require.Contains(t, fragmentLines, "IdentityFile "+cmdB.fallbackKeyPath) + + // Second identity re-login: same fallback file overwritten in place, no + // duplicate fragment line. + cmdB2 := newCmd(pktB) + require.NoError(t, cmdB2.writeKeysToDestination("", pemB2, certB2)) + require.Equal(t, cmdB.fallbackKeyPath, cmdB2.fallbackKeyPath) + require.Equal(t, pemB2, mustRead(cmdB.fallbackKeyPath)) + fragmentLines = strings.Split(string(mustRead(filepath.Join(opkDirPath, "config"))), "\n") + lineCount := 0 + for _, line := range fragmentLines { + if line == "IdentityFile "+cmdB.fallbackKeyPath { + lineCount++ + } + } + require.Equal(t, 1, lineCount, "fragment must not accumulate duplicate IdentityFile lines") +} + +func TestWriteKeysLegacyCommentFallback(t *testing.T) { + // A cert written by an older opkssh whose embedded PK token no longer + // parses must still be overwritten in place when its comment is exactly + // "openpubkey" — requirement: never demote a single-identity re-login to + // a relocated key on a parse failure. + pkt, signer, _ := Mocks(t, ECDSA) + certBytes, pem, err := createSSHCert(pkt, signer, []string{"test"}) + require.NoError(t, err) + + pub, priv, err := ed25519.GenerateKey(rand.Reader) + require.NoError(t, err) + sshPub, err := ssh.NewPublicKey(pub) + require.NoError(t, err) + sshSigner, err := ssh.NewSignerFromSigner(priv) + require.NoError(t, err) + legacyCert := &ssh.Certificate{ + Key: sshPub, + CertType: ssh.UserCert, + ValidBefore: ssh.CertTimeInfinity, + } + require.NoError(t, legacyCert.SignCert(rand.Reader, sshSigner)) + legacyLine := bytes.TrimSuffix(ssh.MarshalAuthorizedKey(legacyCert), []byte("\n")) + legacyLine = append(legacyLine, []byte(" openpubkey")...) + identity, parsed := certFileIdentity(legacyLine) + require.False(t, parsed, "test premise: the legacy cert must not carry a parseable PK token") + require.Zero(t, identity) + + homePath, err := os.UserHomeDir() + require.NoError(t, err) + defaultKeyPath := filepath.Join(homePath, ".ssh", "id_ecdsa") + + fs := afero.NewMemMapFs() + afs := &afero.Afero{Fs: fs} + require.NoError(t, afs.WriteFile(defaultKeyPath, []byte("legacy-key"), 0o600)) + require.NoError(t, afs.WriteFile(defaultKeyPath+"-cert.pub", legacyLine, 0o644)) + + cmd := &LoginCmd{Fs: fs, KeyTypeArg: ECDSA, pkt: pkt, OutWriter: &bytes.Buffer{}} + require.NoError(t, cmd.writeKeysToDestination("", pem, certBytes)) + require.Empty(t, cmd.fallbackKeyPath) + written, err := afs.ReadFile(defaultKeyPath) + require.NoError(t, err) + require.Equal(t, pem, written, "legacy-comment slot must be overwritten in place") +} + +// seedForeignDefaultSlots fills every default ECDSA key slot with a foreign +// (non-opkssh) key so a login is forced onto the fallback path. +func seedForeignDefaultSlots(t *testing.T, fs afero.Fs) { + homePath, err := os.UserHomeDir() + require.NoError(t, err) + afs := &afero.Afero{Fs: fs} + for _, name := range DefaultSSHKeyFileNames[ECDSA] { + p := filepath.Join(homePath, ".ssh", name) + require.NoError(t, afs.WriteFile(p, []byte("foreign"), 0o600)) + require.NoError(t, afs.WriteFile(p+"-cert.pub", foreignPubkeyLine(t), 0o644)) + } +} + +func TestFallbackWarningDecision(t *testing.T) { + const warningMarker = "will not try" + + t.Run("warns on fallback when no agent holds the key", func(t *testing.T) { + t.Setenv("SSH_AUTH_SOCK", "") + _, _, mockOp := Mocks(t, ECDSA) + fs := afero.NewMemMapFs() + seedForeignDefaultSlots(t, fs) + out := &bytes.Buffer{} + l := &LoginCmd{Fs: fs, KeyTypeArg: ECDSA, OutWriter: out} + require.NoError(t, l.Login(context.Background(), mockOp, false, "")) + require.Contains(t, out.String(), warningMarker) + homePath, err := os.UserHomeDir() + require.NoError(t, err) + require.Contains(t, out.String(), filepath.Join(homePath, ".ssh", "opkssh"), + "the warning must name the fallback location it is about") + }) + + t.Run("silent on fallback when the agent add succeeds", func(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("test agent listens on a unix domain socket") + } + sockPath := filepath.Join(t.TempDir(), "agent.sock") + listener, err := net.Listen("unix", sockPath) + require.NoError(t, err) + defer listener.Close() + go func() { + conn, err := listener.Accept() + if err != nil { + return + } + _ = agent.ServeAgent(agent.NewKeyring(), conn) + }() + t.Setenv("SSH_AUTH_SOCK", sockPath) + + _, _, mockOp := Mocks(t, ECDSA) + fs := afero.NewMemMapFs() + seedForeignDefaultSlots(t, fs) + out := &bytes.Buffer{} + l := &LoginCmd{Fs: fs, KeyTypeArg: ECDSA, OutWriter: out} + require.NoError(t, l.Login(context.Background(), mockOp, false, "")) + require.Contains(t, out.String(), "Certificate added to ssh-agent") + require.NotContains(t, out.String(), warningMarker) + }) + + t.Run("silent on default-slot writes", func(t *testing.T) { + t.Setenv("SSH_AUTH_SOCK", "") + _, _, mockOp := Mocks(t, ECDSA) + out := &bytes.Buffer{} + l := &LoginCmd{Fs: afero.NewMemMapFs(), KeyTypeArg: ECDSA, OutWriter: out} + require.NoError(t, l.Login(context.Background(), mockOp, false, "")) + require.NotContains(t, out.String(), warningMarker) + }) +} + +func TestOpkSSHDirPartialStateDisambiguates(t *testing.T) { + // A private key without its certificate cannot prove any identity, so + // the slot must be treated as foreign and left untouched. + pkt, signer, _ := Mocks(t, ECDSA) + certBytes, pem, err := createSSHCert(pkt, signer, []string{"test"}) + require.NoError(t, err) + identity, ok := pktIdentity(pkt) + require.True(t, ok) + + homePath, err := os.UserHomeDir() + require.NoError(t, err) + opkDirPath := filepath.Join(homePath, ".ssh", "opkssh") + + fs := afero.NewMemMapFs() + afs := &afero.Afero{Fs: fs} + cmd := &LoginCmd{Fs: fs, KeyTypeArg: ECDSA, pkt: pkt, OutWriter: &bytes.Buffer{}} + + basePath := filepath.Join(opkDirPath, cmd.makeSSHKeyFileName(pkt)) + require.NoError(t, afs.WriteFile(basePath, []byte("orphan"), 0o600)) + + written, err := cmd.writeKeysToOpkSSHDir(pem, certBytes) + require.NoError(t, err) + require.Equal(t, basePath+"-"+identityTag(identity), written) + orphan, err := afs.ReadFile(basePath) + require.NoError(t, err) + require.Equal(t, []byte("orphan"), orphan, "orphaned private key must be untouched") +} + +func TestFragmentWholeLineDedup(t *testing.T) { + // The discriminating case for whole-line matching: the fragment already + // holds the TAGGED path's line, whose text contains the base path as a + // prefix. A base-path write must still add its own line — a substring + // check would wrongly skip it. + pkt, signer, _ := Mocks(t, ECDSA) + certBytes, pem, err := createSSHCert(pkt, signer, []string{"test"}) + require.NoError(t, err) + identity, ok := pktIdentity(pkt) + require.True(t, ok) + + homePath, err := os.UserHomeDir() + require.NoError(t, err) + opkDirPath := filepath.Join(homePath, ".ssh", "opkssh") + + fs := afero.NewMemMapFs() + afs := &afero.Afero{Fs: fs} + cmd := &LoginCmd{Fs: fs, KeyTypeArg: ECDSA, pkt: pkt, OutWriter: &bytes.Buffer{}} + + basePath := filepath.Join(opkDirPath, cmd.makeSSHKeyFileName(pkt)) + taggedLine := "IdentityFile " + basePath + "-" + identityTag(identity) + require.NoError(t, afs.WriteFile(filepath.Join(opkDirPath, "config"), []byte(taggedLine+"\n"), 0o600)) + + written, err := cmd.writeKeysToOpkSSHDir(pem, certBytes) + require.NoError(t, err) + require.Equal(t, basePath, written, "free base slot must be used") + + fragmentLines := strings.Split(string(mustReadFile(t, afs, filepath.Join(opkDirPath, "config"))), "\n") + require.Contains(t, fragmentLines, "IdentityFile "+basePath, "base line must be added despite being a substring of the tagged line") + require.Contains(t, fragmentLines, taggedLine, "pre-existing tagged line must be preserved") +} + +func mustReadFile(t *testing.T, afs *afero.Afero, path string) []byte { + b, err := afs.ReadFile(path) + require.NoError(t, err) + return b +} + +func TestOpkSSHDirForeignTaggedPathErrors(t *testing.T) { + // The clobber-protection must hold at the tagged path too: a foreign + // occupant there is an error, never a silent overwrite. + pkt, signer, _ := Mocks(t, ECDSA) + certBytes, pem, err := createSSHCert(pkt, signer, []string{"test"}) + require.NoError(t, err) + identity, ok := pktIdentity(pkt) + require.True(t, ok) + + homePath, err := os.UserHomeDir() + require.NoError(t, err) + opkDirPath := filepath.Join(homePath, ".ssh", "opkssh") + + fs := afero.NewMemMapFs() + afs := &afero.Afero{Fs: fs} + cmd := &LoginCmd{Fs: fs, KeyTypeArg: ECDSA, pkt: pkt, OutWriter: &bytes.Buffer{}} + + baseName := cmd.makeSSHKeyFileName(pkt) + basePath := filepath.Join(opkDirPath, baseName) + taggedPath := filepath.Join(opkDirPath, baseName+"-"+identityTag(identity)) + for _, p := range []string{basePath, taggedPath} { + require.NoError(t, afs.WriteFile(p, []byte("foreign"), 0o600)) + require.NoError(t, afs.WriteFile(p+"-cert.pub", foreignPubkeyLine(t), 0o644)) + } + + _, err = cmd.writeKeysToOpkSSHDir(pem, certBytes) + require.ErrorContains(t, err, "holds a different identity") +} diff --git a/docs/cli/opkssh.md b/docs/cli/opkssh.md index 1a3d9d0a..98dd500d 100644 --- a/docs/cli/opkssh.md +++ b/docs/cli/opkssh.md @@ -40,4 +40,4 @@ opkssh [flags] * [opkssh readhome](opkssh_readhome.md) - Read the principal's home policy file * [opkssh verify](opkssh_verify.md) - Verify an SSH key (used by sshd AuthorizedKeysCommand) -###### Auto generated by spf13/cobra on 23-Jun-2026 +###### Auto generated by spf13/cobra on 20-Aug-2026 diff --git a/docs/cli/opkssh_add.md b/docs/cli/opkssh_add.md index b32608a2..18d581f8 100644 --- a/docs/cli/opkssh_add.md +++ b/docs/cli/opkssh_add.md @@ -36,4 +36,4 @@ opkssh add [flags] * [opkssh](opkssh.md) - SSH with OpenPubkey -###### Auto generated by spf13/cobra on 23-Jun-2026 +###### Auto generated by spf13/cobra on 20-Aug-2026 diff --git a/docs/cli/opkssh_audit.md b/docs/cli/opkssh_audit.md index 17da2958..1ebd7019 100644 --- a/docs/cli/opkssh_audit.md +++ b/docs/cli/opkssh_audit.md @@ -42,4 +42,4 @@ opkssh audit [flags] * [opkssh](opkssh.md) - SSH with OpenPubkey -###### Auto generated by spf13/cobra on 23-Jun-2026 +###### Auto generated by spf13/cobra on 20-Aug-2026 diff --git a/docs/cli/opkssh_client.md b/docs/cli/opkssh_client.md index e33ac3bb..9372a5d3 100644 --- a/docs/cli/opkssh_client.md +++ b/docs/cli/opkssh_client.md @@ -19,4 +19,4 @@ Interact with client configuration * [opkssh](opkssh.md) - SSH with OpenPubkey * [opkssh client provider](opkssh_client_provider.md) - Interact with provider configuration -###### Auto generated by spf13/cobra on 23-Jun-2026 +###### Auto generated by spf13/cobra on 20-Aug-2026 diff --git a/docs/cli/opkssh_client_provider.md b/docs/cli/opkssh_client_provider.md index 91b8b69a..9fd8596e 100644 --- a/docs/cli/opkssh_client_provider.md +++ b/docs/cli/opkssh_client_provider.md @@ -19,4 +19,4 @@ Interact with provider configuration * [opkssh client](opkssh_client.md) - Interact with client configuration * [opkssh client provider list](opkssh_client_provider_list.md) - List configured providers -###### Auto generated by spf13/cobra on 23-Jun-2026 +###### Auto generated by spf13/cobra on 20-Aug-2026 diff --git a/docs/cli/opkssh_client_provider_list.md b/docs/cli/opkssh_client_provider_list.md index 7832f57f..4b05b34c 100644 --- a/docs/cli/opkssh_client_provider_list.md +++ b/docs/cli/opkssh_client_provider_list.md @@ -15,7 +15,7 @@ opkssh client provider list [flags] ### Options ``` - --config-path string Path to the client config file. Default: ~/.opk/config.yml on linux and %APPDATA%\.opk\config.yml on windows. + --config-path string Path to the client config file. Default: the first existing of $XDG_CONFIG_HOME/opk/config.yml (~/.config/opk/config.yml on linux/macOS, %AppData%\opk\config.yml on windows) and the legacy ~/.opk/config.yml. -h, --help help for list ``` @@ -23,4 +23,4 @@ opkssh client provider list [flags] * [opkssh client provider](opkssh_client_provider.md) - Interact with provider configuration -###### Auto generated by spf13/cobra on 23-Jun-2026 +###### Auto generated by spf13/cobra on 20-Aug-2026 diff --git a/docs/cli/opkssh_inspect.md b/docs/cli/opkssh_inspect.md index dff7d9c5..905bc867 100644 --- a/docs/cli/opkssh_inspect.md +++ b/docs/cli/opkssh_inspect.md @@ -22,4 +22,4 @@ opkssh inspect [flags] * [opkssh](opkssh.md) - SSH with OpenPubkey -###### Auto generated by spf13/cobra on 23-Jun-2026 +###### Auto generated by spf13/cobra on 20-Aug-2026 diff --git a/docs/cli/opkssh_login.md b/docs/cli/opkssh_login.md index cd4a8c89..2ccb63d5 100644 --- a/docs/cli/opkssh_login.md +++ b/docs/cli/opkssh_login.md @@ -6,7 +6,7 @@ Authenticate with an OpenID Provider to generate an SSH key for opkssh Login creates opkssh SSH keys -Login generates a key pair, then opens a browser to authenticate the user with the OpenID Provider. Upon successful authentication, opkssh creates an SSH public key (~/.ssh/id_ecdsa) containing the user's PK token. By default, this SSH key expires after 24 hours, after which the user must run "opkssh login" again to generate a new key. +Login generates a key pair, then opens a browser to authenticate the user with the OpenID Provider. Upon successful authentication, opkssh creates an SSH public key (~/.ssh/id_ecdsa) containing the user's PK token. By default, this SSH key expires after 24 hours, after which the user must run "opkssh login" again to generate a new key. When the default key files already hold a different identity, keys for additional identities are written to the opkssh identity directory (~/.ssh/opkssh) instead of overwriting them. Users can then SSH into servers configured to use opkssh as the AuthorizedKeysCommand. The server verifies the PK token and grants access if the token is valid and the user is authorized per the auth_id policy. Arguments: @@ -29,7 +29,7 @@ opkssh login [alias] [flags] ``` --auto-refresh Automatically refresh PK token after login - --config-path string Path to the client config file. Default: ~/.opk/config.yml on linux and %APPDATA%\.opk\config.yml on windows + --config-path string Path to the client config file. Default: the first existing of $XDG_CONFIG_HOME/opk/config.yml (~/.config/opk/config.yml on linux/macOS, %AppData%\opk\config.yml on windows) and the legacy ~/.opk/config.yml --configure Apply changes to ssh config and create ~/.ssh/opkssh directory --create-config Creates a client config file if it does not exist --disable-browser-open Set this flag to disable opening the browser. Useful for choosing the browser you want to use diff --git a/docs/cli/opkssh_logout.md b/docs/cli/opkssh_logout.md index 36f6dfec..681746a6 100644 --- a/docs/cli/opkssh_logout.md +++ b/docs/cli/opkssh_logout.md @@ -33,4 +33,4 @@ opkssh logout [flags] * [opkssh](opkssh.md) - SSH with OpenPubkey -###### Auto generated by spf13/cobra on 23-Jun-2026 +###### Auto generated by spf13/cobra on 20-Aug-2026 diff --git a/docs/cli/opkssh_permissions.md b/docs/cli/opkssh_permissions.md index 11ed5d8b..d0261669 100644 --- a/docs/cli/opkssh_permissions.md +++ b/docs/cli/opkssh_permissions.md @@ -15,4 +15,4 @@ Check and fix filesystem permissions required by opkssh * [opkssh permissions fix](opkssh_permissions_fix.md) - Fix permissions and ownership for opkssh files (requires admin) * [opkssh permissions install](opkssh_permissions_install.md) - Idempotent installer-friendly permissions fix (non-interactive) -###### Auto generated by spf13/cobra on 23-Jun-2026 +###### Auto generated by spf13/cobra on 20-Aug-2026 diff --git a/docs/cli/opkssh_permissions_check.md b/docs/cli/opkssh_permissions_check.md index fbe82cad..a7e2b8cb 100644 --- a/docs/cli/opkssh_permissions_check.md +++ b/docs/cli/opkssh_permissions_check.md @@ -17,4 +17,4 @@ opkssh permissions check [flags] * [opkssh permissions](opkssh_permissions.md) - Check and fix filesystem permissions required by opkssh -###### Auto generated by spf13/cobra on 23-Jun-2026 +###### Auto generated by spf13/cobra on 20-Aug-2026 diff --git a/docs/cli/opkssh_permissions_fix.md b/docs/cli/opkssh_permissions_fix.md index c3074a96..5b3dbf11 100644 --- a/docs/cli/opkssh_permissions_fix.md +++ b/docs/cli/opkssh_permissions_fix.md @@ -20,4 +20,4 @@ opkssh permissions fix [flags] * [opkssh permissions](opkssh_permissions.md) - Check and fix filesystem permissions required by opkssh -###### Auto generated by spf13/cobra on 23-Jun-2026 +###### Auto generated by spf13/cobra on 20-Aug-2026 diff --git a/docs/cli/opkssh_permissions_install.md b/docs/cli/opkssh_permissions_install.md index 6c20863a..99f7fcf6 100644 --- a/docs/cli/opkssh_permissions_install.md +++ b/docs/cli/opkssh_permissions_install.md @@ -18,4 +18,4 @@ opkssh permissions install [flags] * [opkssh permissions](opkssh_permissions.md) - Check and fix filesystem permissions required by opkssh -###### Auto generated by spf13/cobra on 23-Jun-2026 +###### Auto generated by spf13/cobra on 20-Aug-2026 diff --git a/docs/cli/opkssh_readhome.md b/docs/cli/opkssh_readhome.md index 2bf177c2..feeb9b84 100644 --- a/docs/cli/opkssh_readhome.md +++ b/docs/cli/opkssh_readhome.md @@ -29,4 +29,4 @@ opkssh readhome [flags] * [opkssh](opkssh.md) - SSH with OpenPubkey -###### Auto generated by spf13/cobra on 23-Jun-2026 +###### Auto generated by spf13/cobra on 20-Aug-2026 diff --git a/docs/cli/opkssh_verify.md b/docs/cli/opkssh_verify.md index fcd88647..1aa8abc9 100644 --- a/docs/cli/opkssh_verify.md +++ b/docs/cli/opkssh_verify.md @@ -51,4 +51,4 @@ opkssh verify [flags] * [opkssh](opkssh.md) - SSH with OpenPubkey -###### Auto generated by spf13/cobra on 23-Jun-2026 +###### Auto generated by spf13/cobra on 20-Aug-2026 diff --git a/docs/config.md b/docs/config.md index 3ebc1637..ea9d6661 100644 --- a/docs/config.md +++ b/docs/config.md @@ -12,12 +12,22 @@ Our goal is to have an distinct meaning for each column. This way if we want to You can check the correctness of server side config files by running the audit command: `sudo opkssh audit`. -## Client config `~/.opk/config.yml` +## Client config + +The client config configures which OpenID Providers the user can log in with. +It is looked up in the following order, and the first file that exists wins: + +1. `$XDG_CONFIG_HOME/opk/config.yml` when `XDG_CONFIG_HOME` is set (to an + absolute path, per the XDG Base Directory specification), otherwise + `~/.config/opk/config.yml` on Linux/macOS or `%AppData%\opk\config.yml` on + Windows. +2. The legacy `~/.opk/config.yml` (all platforms). An existing legacy config + keeps working unchanged and is never moved. -The config file for the client is saved in `~/.opk/config.yml`. -It configures which OpenID Providers the user can log in with. This file is not required to exist to use opkssh and it is not created by default. -To create it, simple run `~/opkssh login --create-config`. +To create it, simply run `opkssh login --create-config`; a new config is +written to the first location above unless a legacy config already exists. +Run `opkssh login -v` to see which config file is in use. The default client config can be found in [../commands/config/default-client-config.yml](../commands/config/default-client-config.yml). diff --git a/docs/providers/azure.md b/docs/providers/azure.md index 97dae042..52b4b9fe 100644 --- a/docs/providers/azure.md +++ b/docs/providers/azure.md @@ -84,9 +84,9 @@ https://login.microsoftonline.com/{TENANT ID}/v2.0 {CLIENT ID} 12h To test run `opkssh login --provider="https://login.microsoftonline.com/{TENANT ID}/v2.0,{CLIENT ID}"` with the client ID you registered. If this works then server has been setup correctly. -On the client check to see if you have already created a config at `~/.opk/config.yml`. If no config if found, create a config by running `opkssh login --create-config`. +On the client check to see if you have already created a client config file (see [config locations](../config.md#client-config)). If no config if found, create a config by running `opkssh login --create-config`. -Then edit `~/.opk/config.yml` and change the entry for azure to use the client ID and tenant ID from the App Registration. +Then edit your client config file and change the entry for azure to use the client ID and tenant ID from the App Registration. ```yaml - alias: azure microsoft @@ -141,9 +141,9 @@ To add it follow the instructions here: [Configure group claims for applications Message: AADSTS900561: The endpoint only accepts POST requests. Received a GET request. ``` -On the client check to see if you have already created a config at `~/.opk/config.yml`. If no config is found, create a config by running `opkssh login --create-config`. +On the client check to see if you have already created a client config file (see [config locations](../config.md#client-config)). If no config is found, create a config by running `opkssh login --create-config`. -Edit `~/.opk/config.yml` and for the azure provider change `prompt: consent` to `prompt: none` as shown below. +Edit your client config file and for the azure provider change `prompt: consent` to `prompt: none` as shown below. ```yaml - alias: azure microsoft diff --git a/docs/providers/cognito.md b/docs/providers/cognito.md index f2f115af..39a55ae9 100644 --- a/docs/providers/cognito.md +++ b/docs/providers/cognito.md @@ -122,9 +122,9 @@ https://cognito-idp..amazonaws.com/ 12h To test run `opkssh login --provider="https://cognito-idp.us-east-1.amazonaws.com/us-east-1_xxxxxxxxx,"` with the User Pool ID and Client ID you registered. If this works then the App has been setup correctly. -On the client check to see if you have already created a config at `~/.opk/config.yml`. If no config if found, create a config by running `opkssh login --create-config`. +On the client check to see if you have already created a client config file (see [config locations](../config.md#client-config)). If no config if found, create a config by running `opkssh login --create-config`. -Then edit `~/.opk/config.yml` and add an entry for Cognito to use the User Pool and Client ID from the App Registration. +Then edit your client config file and add an entry for Cognito to use the User Pool and Client ID from the App Registration. ```yaml - alias: cognito diff --git a/docs/providers/keycloak.md b/docs/providers/keycloak.md index 24821d85..bfbd8d18 100644 --- a/docs/providers/keycloak.md +++ b/docs/providers/keycloak.md @@ -58,9 +58,9 @@ https://your-keycloak-instance.tld/realms/your-realm {CLIENT_ID} 24h To test, run `opkssh login --provider="https://your-keycloak-instance.tld/realms/your-realm,CLIENT_ID"` with the `CLIENT_ID` you registered (`opkssh` in this guide). If this works, the server has been set up correctly. -On the client, check whether you have already created a config at `~/.opk/config.yml`. If no config is found, create one by running `opkssh login --create-config`. +On the client, check whether you have already created a client config file (see [config locations](../config.md#client-config)). If no config is found, create one by running `opkssh login --create-config`. -Then edit `~/.opk/config.yml` and change (or add) the `keycloak` provider entry to use the `issuer` (realm URL) and `CLIENT_ID` values from the client you registered in Keycloak. +Then edit your client config file and change (or add) the `keycloak` provider entry to use the `issuer` (realm URL) and `CLIENT_ID` values from the client you registered in Keycloak. ```yaml - alias: keycloak diff --git a/main.go b/main.go index ec55ff55..d7b83a80 100644 --- a/main.go +++ b/main.go @@ -167,7 +167,7 @@ Arguments: Short: "Authenticate with an OpenID Provider to generate an SSH key for opkssh", Long: `Login creates opkssh SSH keys -Login generates a key pair, then opens a browser to authenticate the user with the OpenID Provider. Upon successful authentication, opkssh creates an SSH public key (~/.ssh/id_ecdsa) containing the user's PK token. By default, this SSH key expires after 24 hours, after which the user must run "opkssh login" again to generate a new key. +Login generates a key pair, then opens a browser to authenticate the user with the OpenID Provider. Upon successful authentication, opkssh creates an SSH public key (~/.ssh/id_ecdsa) containing the user's PK token. By default, this SSH key expires after 24 hours, after which the user must run "opkssh login" again to generate a new key. When the default key files already hold a different identity, keys for additional identities are written to the opkssh identity directory (~/.ssh/opkssh) instead of overwriting them. Users can then SSH into servers configured to use opkssh as the AuthorizedKeysCommand. The server verifies the PK token and grants access if the token is valid and the user is authorized per the auth_id policy. Arguments: @@ -209,7 +209,7 @@ Arguments: // Define flags for login. loginCmd.Flags().BoolVar(&autoRefreshArg, "auto-refresh", false, "Automatically refresh PK token after login") - loginCmd.Flags().StringVar(&configPathArg, "config-path", "", "Path to the client config file. Default: ~/.opk/config.yml on linux and %APPDATA%\\.opk\\config.yml on windows") + loginCmd.Flags().StringVar(&configPathArg, "config-path", "", "Path to the client config file. Default: the first existing of $XDG_CONFIG_HOME/opk/config.yml (~/.config/opk/config.yml on linux/macOS, %AppData%\\opk\\config.yml on windows) and the legacy ~/.opk/config.yml") loginCmd.Flags().BoolVar(&createConfigArg, "create-config", false, "Creates a client config file if it does not exist") loginCmd.Flags().BoolVar(&configureArg, "configure", false, "Apply changes to ssh config and create ~/.ssh/opkssh directory") loginCmd.Flags().StringVar(&logDirArg, "log-dir", "", "Directory to write output logs") @@ -482,7 +482,7 @@ Exit code: 0 if all entries are valid, 1 if any warnings or errors are found.`, }, } - providerListCmd.Flags().StringVar(&configPathArg, "config-path", "", "Path to the client config file. Default: ~/.opk/config.yml on linux and %APPDATA%\\.opk\\config.yml on windows.") + providerListCmd.Flags().StringVar(&configPathArg, "config-path", "", "Path to the client config file. Default: the first existing of $XDG_CONFIG_HOME/opk/config.yml (~/.config/opk/config.yml on linux/macOS, %AppData%\\opk\\config.yml on windows) and the legacy ~/.opk/config.yml.") providerCmd.AddCommand(providerListCmd) From 0e60a3faed7be8bb7bab1a966677a6c1ab01387e Mon Sep 17 00:00:00 2001 From: Jason Bowman Date: Thu, 20 Aug 2026 18:26:31 -0700 Subject: [PATCH 08/13] refactor: unify key-slot policy, share the fragment format, trim churn MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Post-review cleanup of the storage-layout change; behavior-preserving: - One slot classifier (classifySlot) now answers "who holds this key-file slot" for every tier. The default tier and the opkssh identity directory keep their distinct write policies as explicit mappings over the classified states; previously two similar loops re-implemented the same pipeline and their one behavioral divergence (an absent private key makes a default slot writable even over an orphaned certificate, while the identity directory fails closed) was undocumented. - keyIdentity now carries its own validity, and equality is only reachable through sameAs, which requires both sides valid — the fail-closed rule is enforced by construction instead of at each comparison site. The identity is extracted once per login and passed down. - The IdentityFile fragment format is one contract shared by login (writer) and logout (remover) via identityFileLine/fragmentLines; the two inline copies could drift, and the writer's matcher did not tolerate CRLF while the remover did. Re-logins no longer rewrite an unchanged fragment. - ResolveClientConfigPath reports whether a config was found, removing the caller's redundant re-stat; the --config-path help text is a shared constant across both commands that carry the flag; candidate enumeration is unexported to keep the module's public API flat. - The fallback landing is returned through the write-path call chain instead of being smuggled through a LoginCmd field. - Tests share fixtures (opkDirFixture, startTestAgent, mustReadFile) and gain found-flag expectations; 13 docs/cli files whose only change was the generated date stamp are dropped from the change. --- commands/config/client_config.go | 37 +-- commands/config/client_config_test.go | 90 ++++---- commands/login.go | 305 +++++++++++++------------ commands/login_test.go | 240 +++++++++---------- commands/logout.go | 9 +- docs/cli/opkssh.md | 2 +- docs/cli/opkssh_add.md | 2 +- docs/cli/opkssh_audit.md | 2 +- docs/cli/opkssh_client.md | 2 +- docs/cli/opkssh_client_provider.md | 2 +- docs/cli/opkssh_inspect.md | 2 +- docs/cli/opkssh_login.md | 2 +- docs/cli/opkssh_logout.md | 2 +- docs/cli/opkssh_permissions.md | 2 +- docs/cli/opkssh_permissions_check.md | 2 +- docs/cli/opkssh_permissions_fix.md | 2 +- docs/cli/opkssh_permissions_install.md | 2 +- docs/cli/opkssh_readhome.md | 2 +- docs/cli/opkssh_verify.md | 2 +- main.go | 4 +- 20 files changed, 362 insertions(+), 351 deletions(-) diff --git a/commands/config/client_config.go b/commands/config/client_config.go index 866d8805..a11648dd 100644 --- a/commands/config/client_config.go +++ b/commands/config/client_config.go @@ -60,7 +60,11 @@ func (c *ClientConfig) GetByIssuer(issuer string) (*ProviderConfig, bool) { return nil, false } -// ClientConfigCandidatePaths returns the client config locations in +// ConfigPathFlagHelp documents the --config-path default resolution chain; +// shared by every command that carries the flag so the copies cannot drift. +const ConfigPathFlagHelp = "Path to the client config file. Default: the first existing of $XDG_CONFIG_HOME/opk/config.yml (~/.config/opk/config.yml on linux/macOS, %AppData%\\opk\\config.yml on windows) and the legacy ~/.opk/config.yml." + +// clientConfigCandidatePaths returns the client config locations in // resolution order: // // 1. /opk/config.yml, where is $XDG_CONFIG_HOME when @@ -69,7 +73,7 @@ func (c *ClientConfig) GetByIssuer(issuer string) (*ProviderConfig, bool) { // systems, %AppData% on Windows). Replacement semantics per the spec: a // set variable replaces the platform default, it does not stack with it. // 2. The legacy ~/.opk/config.yml. -func ClientConfigCandidatePaths() ([]string, error) { +func clientConfigCandidatePaths() ([]string, error) { var configDir string var platformDirErr error if xdgDir := os.Getenv("XDG_CONFIG_HOME"); xdgDir != "" && filepath.IsAbs(xdgDir) { @@ -98,34 +102,37 @@ func ClientConfigCandidatePaths() ([]string, error) { return candidates, nil } -// ResolveClientConfigPath resolves the client config path. An explicitly -// provided path is used as-is. Otherwise the first existing candidate wins — -// so a legacy ~/.opk/config.yml keeps working untouched — and when no config -// exists anywhere the path falls to the first candidate, the XDG-preferred +// ResolveClientConfigPath resolves the client config path and reports +// whether a config file exists there. An explicitly provided path is used +// as-is. Otherwise the first existing candidate wins — so a legacy +// ~/.opk/config.yml keeps working untouched — and when no config exists +// anywhere the path falls to the first candidate, the XDG-preferred // location, which is where a new config is then created. -func ResolveClientConfigPath(fs afero.Fs, configPath *string) error { +func ResolveClientConfigPath(fs afero.Fs, configPath *string) (bool, error) { + afs := &afero.Afero{Fs: fs} if *configPath != "" { - return nil + found, err := afs.Exists(*configPath) + return err == nil && found, nil } - candidates, err := ClientConfigCandidatePaths() + candidates, err := clientConfigCandidatePaths() if err != nil { - return err + return false, err } - afs := &afero.Afero{Fs: fs} for _, candidate := range candidates { if exists, err := afs.Exists(candidate); err == nil && exists { *configPath = candidate - return nil + return true, nil } } *configPath = candidates[0] - return nil + return false, nil } // GetClientConfigFromFile retrieves the client config from the configuration file at configPath. -// If configPath is not specified then the default configuration path is uses ~/.opk/config.yml +// If configPath is not specified it is resolved via ResolveClientConfigPath +// (see clientConfigCandidatePaths for the resolution order). func GetClientConfigFromFile(configPath string, Fs afero.Fs) (*ClientConfig, error) { - if err := ResolveClientConfigPath(Fs, &configPath); err != nil { + if _, err := ResolveClientConfigPath(Fs, &configPath); err != nil { return nil, err } diff --git a/commands/config/client_config_test.go b/commands/config/client_config_test.go index 395730fe..1bbaab7a 100644 --- a/commands/config/client_config_test.go +++ b/commands/config/client_config_test.go @@ -105,64 +105,74 @@ func TestResolveClientConfigPath(t *testing.T) { legacyPath := home + "/.opk/config.yml" tests := []struct { - name string - xdgEnv string - files []string - explicit string - expected string + name string + xdgEnv string + files []string + explicit string + expected string + wantFound bool }{ { - name: "explicit path is used as-is", - explicit: "/tmp/custom.yml", - files: []string{legacyPath}, - expected: "/tmp/custom.yml", + name: "explicit path is used as-is", + explicit: "/tmp/custom.yml", + files: []string{legacyPath}, + expected: "/tmp/custom.yml", + wantFound: false, }, { - name: "XDG set and file exists there", - xdgEnv: "/xdg-config", - files: []string{xdgPath, legacyPath}, - expected: xdgPath, + name: "XDG set and file exists there", + xdgEnv: "/xdg-config", + files: []string{xdgPath, legacyPath}, + expected: xdgPath, + wantFound: true, }, { - name: "XDG replaces the platform dir, it does not stack", - xdgEnv: "/xdg-config", - files: []string{platformPath}, - expected: xdgPath, // ~/.config is never consulted; no legacy -> chain head + name: "XDG replaces the platform dir, it does not stack", + xdgEnv: "/xdg-config", + files: []string{platformPath}, + expected: xdgPath, // ~/.config is never consulted; no legacy -> chain head + wantFound: false, }, { - name: "relative XDG value is ignored per the spec", - xdgEnv: "relative/dir", - files: []string{platformPath}, - expected: platformPath, + name: "relative XDG value is ignored per the spec", + xdgEnv: "relative/dir", + files: []string{platformPath}, + expected: platformPath, + wantFound: true, }, { - name: "platform dir file wins over legacy", - files: []string{platformPath, legacyPath}, - expected: platformPath, + name: "platform dir file wins over legacy", + files: []string{platformPath, legacyPath}, + expected: platformPath, + wantFound: true, }, { - name: "legacy config keeps working when it is the only one", - files: []string{legacyPath}, - expected: legacyPath, + name: "legacy config keeps working when it is the only one", + files: []string{legacyPath}, + expected: legacyPath, + wantFound: true, }, { // The common upgrade scenario: XDG in the environment, but the // user's only config is the legacy one. - name: "XDG set but only legacy exists: legacy wins", - xdgEnv: "/xdg-config", - files: []string{legacyPath}, - expected: legacyPath, + name: "XDG set but only legacy exists: legacy wins", + xdgEnv: "/xdg-config", + files: []string{legacyPath}, + expected: legacyPath, + wantFound: true, }, { - name: "no config anywhere resolves to the chain head for creation", - files: nil, - expected: platformPath, + name: "no config anywhere resolves to the chain head for creation", + files: nil, + expected: platformPath, + wantFound: false, }, { - name: "no config anywhere with XDG set resolves to the XDG head", - xdgEnv: "/xdg-config", - files: nil, - expected: xdgPath, + name: "no config anywhere with XDG set resolves to the XDG head", + xdgEnv: "/xdg-config", + files: nil, + expected: xdgPath, + wantFound: false, }, } @@ -184,8 +194,10 @@ func TestResolveClientConfigPath(t *testing.T) { } configPath := tt.explicit - require.NoError(t, ResolveClientConfigPath(fs, &configPath)) + found, err := ResolveClientConfigPath(fs, &configPath) + require.NoError(t, err) require.Equal(t, tt.expected, configPath) + require.Equal(t, tt.wantFound, found) }) } } diff --git a/commands/login.go b/commands/login.go index ec3b1168..3df01ae1 100644 --- a/commands/login.go +++ b/commands/login.go @@ -111,10 +111,6 @@ type LoginCmd struct { overrideProvider *providers.OpenIdProvider // Used in tests to override the provider to inject a mock provider // State Config *config.ClientConfig - // fallbackKeyPath is set when the default key slots were all taken by - // other identities and the keys were written to the opkssh identity - // directory instead; used to warn when that key is not reachable. - fallbackKeyPath string // Outputs pkt *pktoken.PKToken @@ -175,10 +171,11 @@ func (l *LoginCmd) Run(ctx context.Context) error { // If the Config has been set in the struct don't replace it. This is useful for testing if l.Config == nil { - if err := config.ResolveClientConfigPath(l.Fs, &l.ConfigPathArg); err != nil { + configFound, err := config.ResolveClientConfigPath(l.Fs, &l.ConfigPathArg) + if err != nil { return err } - if _, err := l.Fs.Stat(l.ConfigPathArg); err == nil { + if configFound { if l.CreateConfigArg { log.Printf("--create-config=true but config file already exists at %s", l.ConfigPathArg) } @@ -188,7 +185,7 @@ func (l *LoginCmd) Run(ctx context.Context) error { } else { l.Config = client_config // Multiple config locations are possible (see - // config.ClientConfigCandidatePaths); naming the winner + // config.ResolveClientConfigPath); naming the winner // defuses silent shadowing, e.g. a hand-created legacy file // losing to an existing XDG-location config. if l.Verbosity >= 1 { @@ -580,12 +577,15 @@ func (l *LoginCmd) login(ctx context.Context, provider providers.OpenIdProvider, return nil, fmt.Errorf("failed to generate SSH cert: %w", err) } - // Write ssh secret key and public key to filesystem + // Write ssh secret key and public key to filesystem. fallbackKeyPath is + // set when the default key slots were all taken by other identities and + // the keys landed in the opkssh identity directory instead. + var fallbackKeyPath string if l.PrintKeyArg { w := l.out() fmt.Fprintln(w, string(certBytes)) // Base64 encoded SSH cert fmt.Fprintln(w, string(seckeySshPem)) // SSH private key in OpenSSH native format - } else if err := l.writeKeysToDestination(seckeyPath, seckeySshPem, certBytes); err != nil { + } else if fallbackKeyPath, err = l.writeKeysToDestination(seckeyPath, seckeySshPem, certBytes); err != nil { return nil, err } @@ -596,8 +596,8 @@ func (l *LoginCmd) login(ctx context.Context, provider providers.OpenIdProvider, // written to the fallback location are not tried by ssh on their // own, so the user must hear about it exactly when no agent ended up // holding the key either. - if l.fallbackKeyPath != "" && !addedToAgent { - fmt.Fprintf(l.out(), "warning: ssh will not try %s automatically and no ssh-agent holds it; add an IdentityFile entry for it, use -i, or run `opkssh login --configure`\n", l.fallbackKeyPath) + if fallbackKeyPath != "" && !addedToAgent { + fmt.Fprintf(l.out(), "warning: ssh will not try %s automatically and no ssh-agent holds it; add an IdentityFile entry for it, use -i, or run `opkssh login --configure`\n", fallbackKeyPath) } } @@ -687,7 +687,7 @@ func (l *LoginCmd) LoginWithRefresh(ctx context.Context, provider providers.Refr } // Write ssh secret key and public key to filesystem - if err := l.writeKeysToDestination(seckeyPath, seckeySshPem, certBytes); err != nil { + if _, err := l.writeKeysToDestination(seckeyPath, seckeySshPem, certBytes); err != nil { return err } @@ -889,29 +889,30 @@ func createSSHCertWithAccessToken(pkt *pktoken.PKToken, accessToken []byte, sign // The --private-key-file argument always has the highest precedence so that an // explicitly requested path is never overridden by the opkssh identity // directory or the default location. See https://github.com/openpubkey/opkssh/issues/524 -func (l *LoginCmd) writeKeysToDestination(seckeyPath string, seckeySshPem []byte, certBytes []byte) error { +func (l *LoginCmd) writeKeysToDestination(seckeyPath string, seckeySshPem []byte, certBytes []byte) (string, error) { switch { case seckeyPath != "": // If we have set seckeyPath then write it there if err := l.writeKeys(seckeyPath, seckeyPath+"-cert.pub", seckeySshPem, certBytes); err != nil { - return fmt.Errorf("failed to write SSH keys to filesystem: %w", err) + return "", fmt.Errorf("failed to write SSH keys to filesystem: %w", err) } - return nil + return "", nil case l.SSHConfigured: - if _, err := l.writeKeysToOpkSSHDir(seckeySshPem, certBytes); err != nil { - return fmt.Errorf("failed to write SSH keys to OPK SSH dir: %w", err) + if _, err := l.writeKeysToOpkSSHDir(seckeySshPem, certBytes, pktIdentity(l.pkt)); err != nil { + return "", fmt.Errorf("failed to write SSH keys to OPK SSH dir: %w", err) } - return nil + return "", nil default: // If keyPath isn't set then write it to the default location - if err := l.writeKeysToSSHDir(seckeySshPem, certBytes); err != nil { - return fmt.Errorf("failed to write SSH keys to filesystem: %w", err) + fallbackKeyPath, err := l.writeKeysToSSHDir(seckeySshPem, certBytes, pktIdentity(l.pkt)) + if err != nil { + return "", fmt.Errorf("failed to write SSH keys to filesystem: %w", err) } - return nil + return fallbackKeyPath, nil } } -func (l *LoginCmd) writeKeysToOpkSSHDir(secKeyPem []byte, certBytes []byte) (string, error) { +func (l *LoginCmd) writeKeysToOpkSSHDir(secKeyPem []byte, certBytes []byte, identity keyIdentity) (string, error) { const ( opkSshPath = ".ssh/opkssh" @@ -936,30 +937,30 @@ func (l *LoginCmd) writeKeysToOpkSSHDir(secKeyPem []byte, certBytes []byte) (str } afs := &afero.Afero{Fs: l.Fs} - identity, identityOK := pktIdentity(l.pkt) sshKeyName := l.makeSSHKeyFileName(l.pkt) privKeyPath := filepath.Join(opkSshUserPath, sshKeyName) pubKeyPath := privKeyPath + "-cert.pub" // A different account at the same provider maps to the same - // - file name; it must not be clobbered. The slot is - // reusable only when the existing certificate provably belongs to the - // same identity; otherwise (different identity, unparseable, or a - // private key without its certificate) disambiguate with an identity - // tag — and apply the same rule at the tagged path, erroring rather - // than silently replacing a foreign occupant. - if !l.slotReusable(privKeyPath, pubKeyPath, identity, identityOK) { + // - file name; it must not be clobbered. Policy for + // this tier: a slot is writable only when it is fully free or the + // existing certificate provably belongs to the same identity; anything + // else (different identity, unparseable, or a private key without its + // certificate) disambiguates with an identity tag — and the same rule + // applies at the tagged path, erroring rather than silently replacing a + // foreign occupant. + if !dirSlotWritable(classifySlot(l.Fs, privKeyPath, pubKeyPath, identity)) { // Tagging a nothing would produce a stable but non-identifying file // name; failing loudly is clearer (unreachable in practice — the PK // token just completed a login). - if !identityOK { + if !identity.valid { return "", errors.New("cannot disambiguate the key file name: no identity could be extracted from the PK token") } sshKeyName = sshKeyName + "-" + identityTag(identity) privKeyPath = filepath.Join(opkSshUserPath, sshKeyName) pubKeyPath = privKeyPath + "-cert.pub" - if !l.slotReusable(privKeyPath, pubKeyPath, identity, identityOK) { + if !dirSlotWritable(classifySlot(l.Fs, privKeyPath, pubKeyPath, identity)) { return "", fmt.Errorf("key file %s exists and holds a different identity; remove it or use -i to pick another path", privKeyPath) } } @@ -972,7 +973,10 @@ func (l *LoginCmd) writeKeysToOpkSSHDir(secKeyPem []byte, certBytes []byte) (str return "", err } - // add key to config + // add key to config. Whole-line match (a tagged path contains its + // untagged prefix, so a substring check would wrongly skip needed + // entries); when the line is already present the fragment is left + // entirely untouched. configContent, err := afs.ReadFile(opkSshConfigPath) if err != nil { if !os.IsNotExist(err) { @@ -980,113 +984,70 @@ func (l *LoginCmd) writeKeysToOpkSSHDir(secKeyPem []byte, certBytes []byte) (str } configContent = nil } - - // Whole-line match: a tagged path contains its untagged prefix, so a - // substring check would wrongly skip needed entries. - identityLine := "IdentityFile " + privKeyPath - hasLine := false - for _, line := range strings.Split(string(configContent), "\n") { - if strings.TrimSpace(line) == identityLine { - hasLine = true - break - } - } + identityLine := identityFileLine(privKeyPath) + hasLine := slices.ContainsFunc(fragmentLines(configContent), func(line string) bool { + return strings.TrimSpace(line) == identityLine + }) if !hasLine { - configContent = slices.Concat( - []byte(identityLine+"\n"), - configContent, - ) - } - - err = afs.WriteFile(opkSshConfigPath, configContent, 0600) - if err != nil { - return "", fmt.Errorf("failed to write opk ssh config file (%s): %w", opkSshConfigPath, err) + configContent = slices.Concat([]byte(identityLine+"\n"), configContent) + if err := afs.WriteFile(opkSshConfigPath, configContent, 0600); err != nil { + return "", fmt.Errorf("failed to write opk ssh config file (%s): %w", opkSshConfigPath, err) + } } return privKeyPath, nil } -// slotReusable reports whether a key file slot may be written for the given -// identity: the slot is free, or its certificate provably belongs to the -// same identity. Identity comparison fails closed — an unparseable existing -// certificate or an unknown own identity never authorizes an overwrite. -func (l *LoginCmd) slotReusable(privKeyPath, pubKeyPath string, identity keyIdentity, identityOK bool) bool { - if !l.fileExists(privKeyPath) && !l.fileExists(pubKeyPath) { +// dirSlotWritable is the opkssh identity directory's write policy over +// classified slots: only a fully free slot or a provably same-identity +// certificate may be written; every other state disambiguates or errors. +func dirSlotWritable(info slotInfo) bool { + if info.cert == certSameIdentity { return true } - if !l.fileExists(pubKeyPath) { - return false - } - existing, err := (&afero.Afero{Fs: l.Fs}).ReadFile(pubKeyPath) - if err != nil { - return false - } - existingIdentity, parsed := certFileIdentity(existing) - return parsed && identityOK && existingIdentity == identity + return info.cert == certAbsent && !info.privExists } -func (l *LoginCmd) writeKeysToSSHDir(seckeySshPem []byte, certBytes []byte) error { +func (l *LoginCmd) writeKeysToSSHDir(seckeySshPem []byte, certBytes []byte, identity keyIdentity) (string, error) { homePath, err := os.UserHomeDir() if err != nil { - return err + return "", err } sshPath := filepath.Join(homePath, ".ssh") // Make ~/.ssh if folder does not exist - err = l.Fs.MkdirAll(sshPath, os.ModePerm) - if err != nil { - return err + if err := l.Fs.MkdirAll(sshPath, os.ModePerm); err != nil { + return "", err } // For ssh to automatically find the key created by openpubkey when - // connecting, we prefer one of the default ssh key paths. A slot is - // reusable when it is free or when it already holds a certificate for - // the SAME identity (a re-login or refresh); a key belonging to a - // different identity — another opkssh account or a foreign key — is - // never overwritten. + // connecting, we prefer one of the default ssh key paths. keyFileNames, ok := DefaultSSHKeyFileNames[l.KeyTypeArg] if !ok { - return fmt.Errorf("key type (%s) has no default output file name; use -i ", l.KeyTypeArg.String()) + return "", fmt.Errorf("key type (%s) has no default output file name; use -i ", l.KeyTypeArg.String()) } - identity, identityOK := pktIdentity(l.pkt) - afs := &afero.Afero{Fs: l.Fs} for _, keyFilename := range keyFileNames { seckeyPath := filepath.Join(sshPath, keyFilename) pubkeyPath := seckeyPath + "-cert.pub" - - if !l.fileExists(seckeyPath) { - // If ssh key file does not currently exist, we don't have to worry about overwriting it - return l.writeKeys(seckeyPath, pubkeyPath, seckeySshPem, certBytes) - } else if !l.fileExists(pubkeyPath) { - continue - } else { - sshPubkey, err := afs.ReadFile(pubkeyPath) - if err != nil { - log.Println("Failed to read:", pubkeyPath) - continue - } - pubkey, comment, _, _, err := ssh.ParseAuthorizedKey(sshPubkey) - if err != nil { - log.Println("Failed to parse:", pubkeyPath) - continue - } - if existingIdentity, parsed := certKeyIdentity(pubkey); parsed { - if identityOK && existingIdentity == identity { - return l.writeKeys(seckeyPath, pubkeyPath, seckeySshPem, certBytes) - } - // A parseable certificate for a different identity is - // protected; try the next slot. - continue - } - // Legacy fallback: an opkssh certificate whose embedded PK token - // no longer parses must still be reusable exactly as before this - // change, which keyed reuse on the comment alone. This repo does - // not control the PK token wire format's history, and demoting a - // re-login to a relocated key on a parse failure would silently - // shadow the fresh key with the stale file. - if comment == "openpubkey" { - return l.writeKeys(seckeyPath, pubkeyPath, seckeySshPem, certBytes) - } + info := classifySlot(l.Fs, seckeyPath, pubkeyPath, identity) + + // Policy for the default tier, preserving pre-change behavior + // exactly: an absent private key makes the slot writable even over + // an orphaned or foreign certificate. With the private key present, + // only a provably same-identity certificate may be overwritten — + // plus the legacy case of a certificate with no extractable PK + // token identity but the comment exactly "openpubkey", which was + // the pre-change reuse condition; this repo does not control the PK + // token wire format's history, and demoting a re-login to a + // relocated key on a parse failure would silently shadow the fresh + // key with the stale file. A parseable certificate for a different + // identity, an orphaned private key, and an unreadable certificate + // are protected. + if !info.privExists || info.cert == certSameIdentity || info.cert == certLegacyComment { + return "", l.writeKeys(seckeyPath, pubkeyPath, seckeySshPem, certBytes) + } + if info.cert == certUnreadable { + log.Println("Failed to read or parse:", pubkeyPath) } } @@ -1095,64 +1056,128 @@ func (l *LoginCmd) writeKeysToSSHDir(seckeySshPem []byte, certBytes []byte) erro // try non-default file names on its own — the key is reachable through // ssh-agent (login loads it there when one is available), an // IdentityFile entry, -i, or the --configure mechanism. - writtenPath, err := l.writeKeysToOpkSSHDir(seckeySshPem, certBytes) - if err != nil { - return err - } - l.fallbackKeyPath = writtenPath - return nil + return l.writeKeysToOpkSSHDir(seckeySshPem, certBytes, identity) } -// keyIdentity is the identity triple a written key file is bound to. +// keyIdentity is the identity triple a written key file is bound to. The +// zero value is invalid; equality is only ever established through sameAs. type keyIdentity struct { issuer string audience string subject string + valid bool +} + +// sameAs reports whether two identities are provably the same. It fails +// closed by construction: an invalid identity — nil token, failed claim +// extraction, unparseable certificate — never compares equal to anything, +// because equality is what authorizes overwriting a key file. +func (k keyIdentity) sameAs(other keyIdentity) bool { + return k.valid && other.valid && k == other } -// pktIdentity extracts the identity triple from a PK token. It fails closed: -// ok is false when the token is nil or any component cannot be extracted, -// and a failed identity must never compare equal to anything — equality is -// what authorizes overwriting a key file. -func pktIdentity(pkt *pktoken.PKToken) (keyIdentity, bool) { +// pktIdentity extracts the identity triple from a PK token, failing closed +// (see sameAs) when the token is nil or any component cannot be extracted. +func pktIdentity(pkt *pktoken.PKToken) keyIdentity { if pkt == nil { - return keyIdentity{}, false + return keyIdentity{} } issuer, errIss := pkt.Issuer() audience, errAud := pkt.Audience() subject, errSub := pkt.Subject() if errIss != nil || errAud != nil || errSub != nil { - return keyIdentity{}, false + return keyIdentity{} } - return keyIdentity{issuer: issuer, audience: audience, subject: subject}, true + return keyIdentity{issuer: issuer, audience: audience, subject: subject, valid: true} } // certFileIdentity extracts the identity from the bytes of a certificate -// file written by opkssh. ok is false when the file does not hold an opkssh -// certificate with an extractable identity (foreign key, plain public key, -// unreadable or unparseable PK token). -func certFileIdentity(certBytes []byte) (keyIdentity, bool) { +// file written by opkssh; invalid when the file does not hold an opkssh +// certificate with an extractable identity. +func certFileIdentity(certBytes []byte) keyIdentity { pubkey, _, _, _, err := ssh.ParseAuthorizedKey(certBytes) if err != nil { - return keyIdentity{}, false + return keyIdentity{} } return certKeyIdentity(pubkey) } // certKeyIdentity extracts the identity from an already-parsed public key. -func certKeyIdentity(pubkey ssh.PublicKey) (keyIdentity, bool) { +func certKeyIdentity(pubkey ssh.PublicKey) keyIdentity { cert, isCert := pubkey.(*ssh.Certificate) if !isCert { - return keyIdentity{}, false + return keyIdentity{} } smuggler := &sshcert.SshCertSmuggler{SshCert: cert} pkt, err := smuggler.GetPKToken() if err != nil { - return keyIdentity{}, false + return keyIdentity{} } return pktIdentity(pkt) } +// certState classifies the -cert.pub side of a key-file slot relative to the +// identity now logging in. +type certState int + +const ( + certAbsent certState = iota + certSameIdentity // provably this identity's certificate + certLegacyComment // no extractable identity, comment exactly "openpubkey" + certForeign // anything else parseable (another identity, or not opkssh's) + certUnreadable // exists but could not be read or parsed +) + +// slotInfo is a classified key-file slot: whether the private key file is +// present, and what the certificate side holds. +type slotInfo struct { + privExists bool + cert certState +} + +// classifySlot answers "who holds this slot?" for a private-key path and its +// -cert.pub companion. It is the mechanism shared by every key-writing tier; +// each tier maps the states to its own write policy. +func classifySlot(fs afero.Fs, privKeyPath, pubKeyPath string, identity keyIdentity) slotInfo { + info := slotInfo{privExists: fsFileExists(fs, privKeyPath)} + if !fsFileExists(fs, pubKeyPath) { + return info + } + certBytes, err := (&afero.Afero{Fs: fs}).ReadFile(pubKeyPath) + if err != nil { + info.cert = certUnreadable + return info + } + pubkey, comment, _, _, err := ssh.ParseAuthorizedKey(certBytes) + if err != nil { + info.cert = certUnreadable + return info + } + existing := certKeyIdentity(pubkey) + switch { + case existing.sameAs(identity): + info.cert = certSameIdentity + case !existing.valid && comment == "openpubkey": + info.cert = certLegacyComment + default: + info.cert = certForeign + } + return info +} + +// identityFileLine renders a config-fragment entry for a private key path. +// The format is a contract between login (which writes entries) and logout +// (which removes them); both sides must go through this function. +func identityFileLine(privKeyPath string) string { + return "IdentityFile " + privKeyPath +} + +// fragmentLines splits config-fragment content into lines, tolerating both +// \n and \r\n endings. +func fragmentLines(fragment []byte) []string { + return strings.Split(strings.ReplaceAll(string(fragment), "\r\n", "\n"), "\n") +} + // commentField preserves the historical key-comment format, which printed // "unknown" for a claim that could not be extracted rather than an empty // field. @@ -1229,10 +1254,10 @@ func (l *LoginCmd) makeSSHKeyFileName(pkt *pktoken.PKToken) string { return keyName } -func (l *LoginCmd) fileExists(fPath string) bool { +func fsFileExists(fs afero.Fs, fPath string) bool { // Stat, not Open: the previous Open-based check leaked a file descriptor // per call. - _, err := l.Fs.Stat(fPath) + _, err := fs.Stat(fPath) return !errors.Is(err, os.ErrNotExist) } diff --git a/commands/login_test.go b/commands/login_test.go index 037e87ab..f2ffe24e 100644 --- a/commands/login_test.go +++ b/commands/login_test.go @@ -664,7 +664,7 @@ func TestAddCertToAgent(t *testing.T) { out := &bytes.Buffer{} l := &LoginCmd{AgentLifetimeArg: "2h", OutWriter: out} - l.addCertToAgent(certBytes, signer) + require.True(t, l.addCertToAgent(certBytes, signer)) require.Contains(t, out.String(), "Certificate added to ssh-agent") mockAgent.mu.Lock() @@ -908,7 +908,7 @@ func TestWriteKeysToDestination(t *testing.T) { pkt: pkt, } - err := l.writeKeysToDestination(seckeyPath, fakeKeyPem, fakeCertBytes) + _, err := l.writeKeysToDestination(seckeyPath, fakeKeyPem, fakeCertBytes) if tt.wantErrContains != "" { require.ErrorContains(t, err, tt.wantErrContains) @@ -1432,15 +1432,20 @@ func TestDetermineProviderWebChooserWithOnlyCICDProviders(t *testing.T) { } func TestIdentityTag(t *testing.T) { - a := keyIdentity{issuer: "https://op.example.com", audience: "client-1", subject: "alice@example.com"} - b := keyIdentity{issuer: "https://op.example.com", audience: "client-1", subject: "bob@example.com"} - c := keyIdentity{issuer: "https://op.example.com", audience: "client-2", subject: "alice@example.com"} + a := keyIdentity{issuer: "https://op.example.com", audience: "client-1", subject: "alice@example.com", valid: true} + b := keyIdentity{issuer: "https://op.example.com", audience: "client-1", subject: "bob@example.com", valid: true} + c := keyIdentity{issuer: "https://op.example.com", audience: "client-2", subject: "alice@example.com", valid: true} require.Len(t, identityTag(a), 8) require.Equal(t, identityTag(a), identityTag(a), "tag must be deterministic") require.NotEqual(t, identityTag(a), identityTag(b), "different subjects must yield different tags") require.NotEqual(t, identityTag(a), identityTag(c), "different audiences must yield different tags") require.NotContains(t, identityTag(a), "alice", "raw subject must not appear in the tag") + + invalid := keyIdentity{issuer: "https://op.example.com", audience: "client-1", subject: "alice@example.com"} + require.False(t, a.sameAs(invalid), "an invalid identity never compares equal") + require.False(t, invalid.sameAs(invalid), "two invalid identities never compare equal") + require.True(t, a.sameAs(a)) } // foreignPubkeyLine returns an authorized-key line for a key opkssh did not @@ -1453,23 +1458,55 @@ func foreignPubkeyLine(t *testing.T) []byte { return ssh.MarshalAuthorizedKey(sshPub) } +// mustReadFile reads a file that the test requires to exist. +func mustReadFile(t *testing.T, afs *afero.Afero, path string) []byte { + t.Helper() + b, err := afs.ReadFile(path) + require.NoError(t, err) + return b +} + +// opkSSHDirPath returns the opkssh identity directory path without creating it. +func opkSSHDirPath(t *testing.T) string { + t.Helper() + homePath, err := os.UserHomeDir() + require.NoError(t, err) + return filepath.Join(homePath, ".ssh", "opkssh") +} + +// opkDirFixture builds the scaffolding the opkssh-identity-dir tests share: a +// mock identity with minted key material and a LoginCmd on a fresh in-memory +// filesystem. +func opkDirFixture(t *testing.T) (cmd *LoginCmd, afs *afero.Afero, dirPath string, identity keyIdentity, pem, certBytes []byte) { + t.Helper() + pkt, signer, _ := Mocks(t, ECDSA) + var err error + certBytes, pem, err = createSSHCert(pkt, signer, []string{"test"}) + require.NoError(t, err) + identity = pktIdentity(pkt) + require.True(t, identity.valid) + fs := afero.NewMemMapFs() + cmd = &LoginCmd{Fs: fs, KeyTypeArg: ECDSA, pkt: pkt, OutWriter: &bytes.Buffer{}} + return cmd, &afero.Afero{Fs: fs}, opkSSHDirPath(t), identity, pem, certBytes +} + func TestWriteKeysIdentitySlots(t *testing.T) { homePath, err := os.UserHomeDir() require.NoError(t, err) defaultKeyPath := filepath.Join(homePath, ".ssh", "id_ecdsa") skKeyPath := filepath.Join(homePath, ".ssh", "id_ecdsa_sk") - opkDirPath := filepath.Join(homePath, ".ssh", "opkssh") + opkDirPath := opkSSHDirPath(t) pktA, signerA, _ := Mocks(t, ECDSA) pktB, signerB, _ := Mocks(t, ECDSA, map[string]any{ "email": "second.identity@example.com", "sub": "second-identity-sub", }) - identityA, okA := pktIdentity(pktA) - identityB, okB := pktIdentity(pktB) - require.True(t, okA) - require.True(t, okB) - require.NotEqual(t, identityA, identityB, "mock identities must differ for this test to mean anything") + identityA := pktIdentity(pktA) + identityB := pktIdentity(pktB) + require.True(t, identityA.valid) + require.True(t, identityB.valid) + require.False(t, identityA.sameAs(identityB), "mock identities must differ for this test to mean anything") certA, pemA, err := createSSHCert(pktA, signerA, []string{"test"}) require.NoError(t, err) @@ -1485,11 +1522,6 @@ func TestWriteKeysIdentitySlots(t *testing.T) { newCmd := func(pkt *pktoken.PKToken) *LoginCmd { return &LoginCmd{Fs: fs, KeyTypeArg: ECDSA, pkt: pkt, OutWriter: &bytes.Buffer{}} } - mustRead := func(path string) []byte { - b, err := afs.ReadFile(path) - require.NoError(t, err) - return b - } // Occupy the id_ecdsa_sk slot with a foreign key so the second identity // exercises the fallback rather than cascading into the _sk slot. @@ -1497,46 +1529,45 @@ func TestWriteKeysIdentitySlots(t *testing.T) { require.NoError(t, afs.WriteFile(skKeyPath+"-cert.pub", foreignPubkeyLine(t), 0o644)) // First login writes the default slot, exactly as before this change. - cmdA := newCmd(pktA) - require.NoError(t, cmdA.writeKeysToDestination("", pemA, certA)) - require.Empty(t, cmdA.fallbackKeyPath) - require.Equal(t, pemA, mustRead(defaultKeyPath)) + fallback, err := newCmd(pktA).writeKeysToDestination("", pemA, certA) + require.NoError(t, err) + require.Empty(t, fallback) + require.Equal(t, pemA, mustReadFile(t, afs, defaultKeyPath)) // Same identity again: overwritten in place, still no fallback, and the // opkssh dir is never created for a single identity. - cmdA2 := newCmd(pktA) - require.NoError(t, cmdA2.writeKeysToDestination("", pemA2, certA2)) - require.Empty(t, cmdA2.fallbackKeyPath) - require.Equal(t, pemA2, mustRead(defaultKeyPath)) + fallback, err = newCmd(pktA).writeKeysToDestination("", pemA2, certA2) + require.NoError(t, err) + require.Empty(t, fallback) + require.Equal(t, pemA2, mustReadFile(t, afs, defaultKeyPath)) dirExists, err := afs.DirExists(opkDirPath) require.NoError(t, err) require.False(t, dirExists, "opkssh dir must not appear while a single identity is in play") // Different identity: the occupied slots are preserved and the keys land // in the opkssh identity directory, with a fragment entry. - cmdB := newCmd(pktB) - require.NoError(t, cmdB.writeKeysToDestination("", pemB, certB)) - require.NotEmpty(t, cmdB.fallbackKeyPath) - require.Equal(t, pemA2, mustRead(defaultKeyPath), "first identity's key must be untouched") - require.Equal(t, []byte("foreign"), mustRead(skKeyPath), "foreign key must be untouched") - require.True(t, strings.HasPrefix(cmdB.fallbackKeyPath, opkDirPath+string(filepath.Separator))) - require.Equal(t, pemB, mustRead(cmdB.fallbackKeyPath)) - writtenIdentity, parsed := certFileIdentity(mustRead(cmdB.fallbackKeyPath + "-cert.pub")) - require.True(t, parsed) - require.Equal(t, identityB, writtenIdentity) - fragmentLines := strings.Split(string(mustRead(filepath.Join(opkDirPath, "config"))), "\n") - require.Contains(t, fragmentLines, "IdentityFile "+cmdB.fallbackKeyPath) + fallbackB, err := newCmd(pktB).writeKeysToDestination("", pemB, certB) + require.NoError(t, err) + require.NotEmpty(t, fallbackB) + require.Equal(t, pemA2, mustReadFile(t, afs, defaultKeyPath), "first identity's key must be untouched") + require.Equal(t, []byte("foreign"), mustReadFile(t, afs, skKeyPath), "foreign key must be untouched") + require.True(t, strings.HasPrefix(fallbackB, opkDirPath+string(filepath.Separator))) + require.Equal(t, pemB, mustReadFile(t, afs, fallbackB)) + writtenIdentity := certFileIdentity(mustReadFile(t, afs, fallbackB+"-cert.pub")) + require.True(t, writtenIdentity.sameAs(identityB)) + configLines := strings.Split(string(mustReadFile(t, afs, filepath.Join(opkDirPath, "config"))), "\n") + require.Contains(t, configLines, identityFileLine(fallbackB)) // Second identity re-login: same fallback file overwritten in place, no // duplicate fragment line. - cmdB2 := newCmd(pktB) - require.NoError(t, cmdB2.writeKeysToDestination("", pemB2, certB2)) - require.Equal(t, cmdB.fallbackKeyPath, cmdB2.fallbackKeyPath) - require.Equal(t, pemB2, mustRead(cmdB.fallbackKeyPath)) - fragmentLines = strings.Split(string(mustRead(filepath.Join(opkDirPath, "config"))), "\n") + fallbackB2, err := newCmd(pktB).writeKeysToDestination("", pemB2, certB2) + require.NoError(t, err) + require.Equal(t, fallbackB, fallbackB2) + require.Equal(t, pemB2, mustReadFile(t, afs, fallbackB)) + configLines = strings.Split(string(mustReadFile(t, afs, filepath.Join(opkDirPath, "config"))), "\n") lineCount := 0 - for _, line := range fragmentLines { - if line == "IdentityFile "+cmdB.fallbackKeyPath { + for _, line := range configLines { + if line == identityFileLine(fallbackB) { lineCount++ } } @@ -1546,8 +1577,8 @@ func TestWriteKeysIdentitySlots(t *testing.T) { func TestWriteKeysLegacyCommentFallback(t *testing.T) { // A cert written by an older opkssh whose embedded PK token no longer // parses must still be overwritten in place when its comment is exactly - // "openpubkey" — requirement: never demote a single-identity re-login to - // a relocated key on a parse failure. + // "openpubkey" — a single-identity re-login must never be demoted to a + // relocated key by a parse failure. pkt, signer, _ := Mocks(t, ECDSA) certBytes, pem, err := createSSHCert(pkt, signer, []string{"test"}) require.NoError(t, err) @@ -1566,9 +1597,8 @@ func TestWriteKeysLegacyCommentFallback(t *testing.T) { require.NoError(t, legacyCert.SignCert(rand.Reader, sshSigner)) legacyLine := bytes.TrimSuffix(ssh.MarshalAuthorizedKey(legacyCert), []byte("\n")) legacyLine = append(legacyLine, []byte(" openpubkey")...) - identity, parsed := certFileIdentity(legacyLine) - require.False(t, parsed, "test premise: the legacy cert must not carry a parseable PK token") - require.Zero(t, identity) + require.False(t, certFileIdentity(legacyLine).valid, + "test premise: the legacy cert must not carry an extractable identity") homePath, err := os.UserHomeDir() require.NoError(t, err) @@ -1580,16 +1610,16 @@ func TestWriteKeysLegacyCommentFallback(t *testing.T) { require.NoError(t, afs.WriteFile(defaultKeyPath+"-cert.pub", legacyLine, 0o644)) cmd := &LoginCmd{Fs: fs, KeyTypeArg: ECDSA, pkt: pkt, OutWriter: &bytes.Buffer{}} - require.NoError(t, cmd.writeKeysToDestination("", pem, certBytes)) - require.Empty(t, cmd.fallbackKeyPath) - written, err := afs.ReadFile(defaultKeyPath) + fallback, err := cmd.writeKeysToDestination("", pem, certBytes) require.NoError(t, err) - require.Equal(t, pem, written, "legacy-comment slot must be overwritten in place") + require.Empty(t, fallback) + require.Equal(t, pem, mustReadFile(t, afs, defaultKeyPath), "legacy-comment slot must be overwritten in place") } // seedForeignDefaultSlots fills every default ECDSA key slot with a foreign // (non-opkssh) key so a login is forced onto the fallback path. func seedForeignDefaultSlots(t *testing.T, fs afero.Fs) { + t.Helper() homePath, err := os.UserHomeDir() require.NoError(t, err) afs := &afero.Afero{Fs: fs} @@ -1604,38 +1634,22 @@ func TestFallbackWarningDecision(t *testing.T) { const warningMarker = "will not try" t.Run("warns on fallback when no agent holds the key", func(t *testing.T) { - t.Setenv("SSH_AUTH_SOCK", "") - _, _, mockOp := Mocks(t, ECDSA) + _, _, mockOp := Mocks(t, ECDSA) // Mocks pins SSH_AUTH_SOCK empty fs := afero.NewMemMapFs() seedForeignDefaultSlots(t, fs) out := &bytes.Buffer{} l := &LoginCmd{Fs: fs, KeyTypeArg: ECDSA, OutWriter: out} require.NoError(t, l.Login(context.Background(), mockOp, false, "")) require.Contains(t, out.String(), warningMarker) - homePath, err := os.UserHomeDir() - require.NoError(t, err) - require.Contains(t, out.String(), filepath.Join(homePath, ".ssh", "opkssh"), + require.Contains(t, out.String(), opkSSHDirPath(t), "the warning must name the fallback location it is about") }) t.Run("silent on fallback when the agent add succeeds", func(t *testing.T) { - if runtime.GOOS == "windows" { - t.Skip("test agent listens on a unix domain socket") - } - sockPath := filepath.Join(t.TempDir(), "agent.sock") - listener, err := net.Listen("unix", sockPath) - require.NoError(t, err) - defer listener.Close() - go func() { - conn, err := listener.Accept() - if err != nil { - return - } - _ = agent.ServeAgent(agent.NewKeyring(), conn) - }() - t.Setenv("SSH_AUTH_SOCK", sockPath) - + // Mocks pins SSH_AUTH_SOCK empty, so the test agent must be started + // after it to win. _, _, mockOp := Mocks(t, ECDSA) + startTestAgent(t, agent.NewKeyring()) fs := afero.NewMemMapFs() seedForeignDefaultSlots(t, fs) out := &bytes.Buffer{} @@ -1646,8 +1660,7 @@ func TestFallbackWarningDecision(t *testing.T) { }) t.Run("silent on default-slot writes", func(t *testing.T) { - t.Setenv("SSH_AUTH_SOCK", "") - _, _, mockOp := Mocks(t, ECDSA) + _, _, mockOp := Mocks(t, ECDSA) // Mocks pins SSH_AUTH_SOCK empty out := &bytes.Buffer{} l := &LoginCmd{Fs: afero.NewMemMapFs(), KeyTypeArg: ECDSA, OutWriter: out} require.NoError(t, l.Login(context.Background(), mockOp, false, "")) @@ -1658,29 +1671,15 @@ func TestFallbackWarningDecision(t *testing.T) { func TestOpkSSHDirPartialStateDisambiguates(t *testing.T) { // A private key without its certificate cannot prove any identity, so // the slot must be treated as foreign and left untouched. - pkt, signer, _ := Mocks(t, ECDSA) - certBytes, pem, err := createSSHCert(pkt, signer, []string{"test"}) - require.NoError(t, err) - identity, ok := pktIdentity(pkt) - require.True(t, ok) - - homePath, err := os.UserHomeDir() - require.NoError(t, err) - opkDirPath := filepath.Join(homePath, ".ssh", "opkssh") - - fs := afero.NewMemMapFs() - afs := &afero.Afero{Fs: fs} - cmd := &LoginCmd{Fs: fs, KeyTypeArg: ECDSA, pkt: pkt, OutWriter: &bytes.Buffer{}} + cmd, afs, dirPath, identity, pem, certBytes := opkDirFixture(t) - basePath := filepath.Join(opkDirPath, cmd.makeSSHKeyFileName(pkt)) + basePath := filepath.Join(dirPath, cmd.makeSSHKeyFileName(cmd.pkt)) require.NoError(t, afs.WriteFile(basePath, []byte("orphan"), 0o600)) - written, err := cmd.writeKeysToOpkSSHDir(pem, certBytes) + written, err := cmd.writeKeysToOpkSSHDir(pem, certBytes, identity) require.NoError(t, err) require.Equal(t, basePath+"-"+identityTag(identity), written) - orphan, err := afs.ReadFile(basePath) - require.NoError(t, err) - require.Equal(t, []byte("orphan"), orphan, "orphaned private key must be untouched") + require.Equal(t, []byte("orphan"), mustReadFile(t, afs, basePath), "orphaned private key must be untouched") } func TestFragmentWholeLineDedup(t *testing.T) { @@ -1688,64 +1687,33 @@ func TestFragmentWholeLineDedup(t *testing.T) { // holds the TAGGED path's line, whose text contains the base path as a // prefix. A base-path write must still add its own line — a substring // check would wrongly skip it. - pkt, signer, _ := Mocks(t, ECDSA) - certBytes, pem, err := createSSHCert(pkt, signer, []string{"test"}) - require.NoError(t, err) - identity, ok := pktIdentity(pkt) - require.True(t, ok) - - homePath, err := os.UserHomeDir() - require.NoError(t, err) - opkDirPath := filepath.Join(homePath, ".ssh", "opkssh") - - fs := afero.NewMemMapFs() - afs := &afero.Afero{Fs: fs} - cmd := &LoginCmd{Fs: fs, KeyTypeArg: ECDSA, pkt: pkt, OutWriter: &bytes.Buffer{}} + cmd, afs, dirPath, identity, pem, certBytes := opkDirFixture(t) - basePath := filepath.Join(opkDirPath, cmd.makeSSHKeyFileName(pkt)) - taggedLine := "IdentityFile " + basePath + "-" + identityTag(identity) - require.NoError(t, afs.WriteFile(filepath.Join(opkDirPath, "config"), []byte(taggedLine+"\n"), 0o600)) + basePath := filepath.Join(dirPath, cmd.makeSSHKeyFileName(cmd.pkt)) + taggedLine := identityFileLine(basePath + "-" + identityTag(identity)) + require.NoError(t, afs.WriteFile(filepath.Join(dirPath, "config"), []byte(taggedLine+"\n"), 0o600)) - written, err := cmd.writeKeysToOpkSSHDir(pem, certBytes) + written, err := cmd.writeKeysToOpkSSHDir(pem, certBytes, identity) require.NoError(t, err) require.Equal(t, basePath, written, "free base slot must be used") - fragmentLines := strings.Split(string(mustReadFile(t, afs, filepath.Join(opkDirPath, "config"))), "\n") - require.Contains(t, fragmentLines, "IdentityFile "+basePath, "base line must be added despite being a substring of the tagged line") - require.Contains(t, fragmentLines, taggedLine, "pre-existing tagged line must be preserved") -} - -func mustReadFile(t *testing.T, afs *afero.Afero, path string) []byte { - b, err := afs.ReadFile(path) - require.NoError(t, err) - return b + configLines := strings.Split(string(mustReadFile(t, afs, filepath.Join(dirPath, "config"))), "\n") + require.Contains(t, configLines, identityFileLine(basePath), "base line must be added despite being a substring of the tagged line") + require.Contains(t, configLines, taggedLine, "pre-existing tagged line must be preserved") } func TestOpkSSHDirForeignTaggedPathErrors(t *testing.T) { // The clobber-protection must hold at the tagged path too: a foreign // occupant there is an error, never a silent overwrite. - pkt, signer, _ := Mocks(t, ECDSA) - certBytes, pem, err := createSSHCert(pkt, signer, []string{"test"}) - require.NoError(t, err) - identity, ok := pktIdentity(pkt) - require.True(t, ok) - - homePath, err := os.UserHomeDir() - require.NoError(t, err) - opkDirPath := filepath.Join(homePath, ".ssh", "opkssh") - - fs := afero.NewMemMapFs() - afs := &afero.Afero{Fs: fs} - cmd := &LoginCmd{Fs: fs, KeyTypeArg: ECDSA, pkt: pkt, OutWriter: &bytes.Buffer{}} + cmd, afs, dirPath, identity, pem, certBytes := opkDirFixture(t) - baseName := cmd.makeSSHKeyFileName(pkt) - basePath := filepath.Join(opkDirPath, baseName) - taggedPath := filepath.Join(opkDirPath, baseName+"-"+identityTag(identity)) + basePath := filepath.Join(dirPath, cmd.makeSSHKeyFileName(cmd.pkt)) + taggedPath := basePath + "-" + identityTag(identity) for _, p := range []string{basePath, taggedPath} { require.NoError(t, afs.WriteFile(p, []byte("foreign"), 0o600)) require.NoError(t, afs.WriteFile(p+"-cert.pub", foreignPubkeyLine(t), 0o644)) } - _, err = cmd.writeKeysToOpkSSHDir(pem, certBytes) + _, err := cmd.writeKeysToOpkSSHDir(pem, certBytes, identity) require.ErrorContains(t, err, "holds a different identity") } diff --git a/commands/logout.go b/commands/logout.go index 49ed4207..befe5559 100644 --- a/commands/logout.go +++ b/commands/logout.go @@ -289,11 +289,10 @@ func (l *LogoutCmd) removeFromOpkSSHConfig(configPath string, seckeyPath string) return nil // Config file doesn't exist, nothing to clean up } - identityLine := "IdentityFile " + seckeyPath - - // Split handling both \r\n (Windows) and \n (Unix) line endings - normalized := strings.ReplaceAll(string(content), "\r\n", "\n") - lines := strings.Split(normalized, "\n") + // identityFileLine and fragmentLines are the shared fragment-format + // contract with login's writer — both sides must stay on these helpers. + identityLine := identityFileLine(seckeyPath) + lines := fragmentLines(content) var newLines []string for _, line := range lines { if strings.TrimSpace(line) != identityLine { diff --git a/docs/cli/opkssh.md b/docs/cli/opkssh.md index 98dd500d..1a3d9d0a 100644 --- a/docs/cli/opkssh.md +++ b/docs/cli/opkssh.md @@ -40,4 +40,4 @@ opkssh [flags] * [opkssh readhome](opkssh_readhome.md) - Read the principal's home policy file * [opkssh verify](opkssh_verify.md) - Verify an SSH key (used by sshd AuthorizedKeysCommand) -###### Auto generated by spf13/cobra on 20-Aug-2026 +###### Auto generated by spf13/cobra on 23-Jun-2026 diff --git a/docs/cli/opkssh_add.md b/docs/cli/opkssh_add.md index 18d581f8..b32608a2 100644 --- a/docs/cli/opkssh_add.md +++ b/docs/cli/opkssh_add.md @@ -36,4 +36,4 @@ opkssh add [flags] * [opkssh](opkssh.md) - SSH with OpenPubkey -###### Auto generated by spf13/cobra on 20-Aug-2026 +###### Auto generated by spf13/cobra on 23-Jun-2026 diff --git a/docs/cli/opkssh_audit.md b/docs/cli/opkssh_audit.md index 1ebd7019..17da2958 100644 --- a/docs/cli/opkssh_audit.md +++ b/docs/cli/opkssh_audit.md @@ -42,4 +42,4 @@ opkssh audit [flags] * [opkssh](opkssh.md) - SSH with OpenPubkey -###### Auto generated by spf13/cobra on 20-Aug-2026 +###### Auto generated by spf13/cobra on 23-Jun-2026 diff --git a/docs/cli/opkssh_client.md b/docs/cli/opkssh_client.md index 9372a5d3..e33ac3bb 100644 --- a/docs/cli/opkssh_client.md +++ b/docs/cli/opkssh_client.md @@ -19,4 +19,4 @@ Interact with client configuration * [opkssh](opkssh.md) - SSH with OpenPubkey * [opkssh client provider](opkssh_client_provider.md) - Interact with provider configuration -###### Auto generated by spf13/cobra on 20-Aug-2026 +###### Auto generated by spf13/cobra on 23-Jun-2026 diff --git a/docs/cli/opkssh_client_provider.md b/docs/cli/opkssh_client_provider.md index 9fd8596e..91b8b69a 100644 --- a/docs/cli/opkssh_client_provider.md +++ b/docs/cli/opkssh_client_provider.md @@ -19,4 +19,4 @@ Interact with provider configuration * [opkssh client](opkssh_client.md) - Interact with client configuration * [opkssh client provider list](opkssh_client_provider_list.md) - List configured providers -###### Auto generated by spf13/cobra on 20-Aug-2026 +###### Auto generated by spf13/cobra on 23-Jun-2026 diff --git a/docs/cli/opkssh_inspect.md b/docs/cli/opkssh_inspect.md index 905bc867..dff7d9c5 100644 --- a/docs/cli/opkssh_inspect.md +++ b/docs/cli/opkssh_inspect.md @@ -22,4 +22,4 @@ opkssh inspect [flags] * [opkssh](opkssh.md) - SSH with OpenPubkey -###### Auto generated by spf13/cobra on 20-Aug-2026 +###### Auto generated by spf13/cobra on 23-Jun-2026 diff --git a/docs/cli/opkssh_login.md b/docs/cli/opkssh_login.md index 2ccb63d5..6af58eff 100644 --- a/docs/cli/opkssh_login.md +++ b/docs/cli/opkssh_login.md @@ -29,7 +29,7 @@ opkssh login [alias] [flags] ``` --auto-refresh Automatically refresh PK token after login - --config-path string Path to the client config file. Default: the first existing of $XDG_CONFIG_HOME/opk/config.yml (~/.config/opk/config.yml on linux/macOS, %AppData%\opk\config.yml on windows) and the legacy ~/.opk/config.yml + --config-path string Path to the client config file. Default: the first existing of $XDG_CONFIG_HOME/opk/config.yml (~/.config/opk/config.yml on linux/macOS, %AppData%\opk\config.yml on windows) and the legacy ~/.opk/config.yml. --configure Apply changes to ssh config and create ~/.ssh/opkssh directory --create-config Creates a client config file if it does not exist --disable-browser-open Set this flag to disable opening the browser. Useful for choosing the browser you want to use diff --git a/docs/cli/opkssh_logout.md b/docs/cli/opkssh_logout.md index 681746a6..36f6dfec 100644 --- a/docs/cli/opkssh_logout.md +++ b/docs/cli/opkssh_logout.md @@ -33,4 +33,4 @@ opkssh logout [flags] * [opkssh](opkssh.md) - SSH with OpenPubkey -###### Auto generated by spf13/cobra on 20-Aug-2026 +###### Auto generated by spf13/cobra on 23-Jun-2026 diff --git a/docs/cli/opkssh_permissions.md b/docs/cli/opkssh_permissions.md index d0261669..11ed5d8b 100644 --- a/docs/cli/opkssh_permissions.md +++ b/docs/cli/opkssh_permissions.md @@ -15,4 +15,4 @@ Check and fix filesystem permissions required by opkssh * [opkssh permissions fix](opkssh_permissions_fix.md) - Fix permissions and ownership for opkssh files (requires admin) * [opkssh permissions install](opkssh_permissions_install.md) - Idempotent installer-friendly permissions fix (non-interactive) -###### Auto generated by spf13/cobra on 20-Aug-2026 +###### Auto generated by spf13/cobra on 23-Jun-2026 diff --git a/docs/cli/opkssh_permissions_check.md b/docs/cli/opkssh_permissions_check.md index a7e2b8cb..fbe82cad 100644 --- a/docs/cli/opkssh_permissions_check.md +++ b/docs/cli/opkssh_permissions_check.md @@ -17,4 +17,4 @@ opkssh permissions check [flags] * [opkssh permissions](opkssh_permissions.md) - Check and fix filesystem permissions required by opkssh -###### Auto generated by spf13/cobra on 20-Aug-2026 +###### Auto generated by spf13/cobra on 23-Jun-2026 diff --git a/docs/cli/opkssh_permissions_fix.md b/docs/cli/opkssh_permissions_fix.md index 5b3dbf11..c3074a96 100644 --- a/docs/cli/opkssh_permissions_fix.md +++ b/docs/cli/opkssh_permissions_fix.md @@ -20,4 +20,4 @@ opkssh permissions fix [flags] * [opkssh permissions](opkssh_permissions.md) - Check and fix filesystem permissions required by opkssh -###### Auto generated by spf13/cobra on 20-Aug-2026 +###### Auto generated by spf13/cobra on 23-Jun-2026 diff --git a/docs/cli/opkssh_permissions_install.md b/docs/cli/opkssh_permissions_install.md index 99f7fcf6..6c20863a 100644 --- a/docs/cli/opkssh_permissions_install.md +++ b/docs/cli/opkssh_permissions_install.md @@ -18,4 +18,4 @@ opkssh permissions install [flags] * [opkssh permissions](opkssh_permissions.md) - Check and fix filesystem permissions required by opkssh -###### Auto generated by spf13/cobra on 20-Aug-2026 +###### Auto generated by spf13/cobra on 23-Jun-2026 diff --git a/docs/cli/opkssh_readhome.md b/docs/cli/opkssh_readhome.md index feeb9b84..2bf177c2 100644 --- a/docs/cli/opkssh_readhome.md +++ b/docs/cli/opkssh_readhome.md @@ -29,4 +29,4 @@ opkssh readhome [flags] * [opkssh](opkssh.md) - SSH with OpenPubkey -###### Auto generated by spf13/cobra on 20-Aug-2026 +###### Auto generated by spf13/cobra on 23-Jun-2026 diff --git a/docs/cli/opkssh_verify.md b/docs/cli/opkssh_verify.md index 1aa8abc9..fcd88647 100644 --- a/docs/cli/opkssh_verify.md +++ b/docs/cli/opkssh_verify.md @@ -51,4 +51,4 @@ opkssh verify [flags] * [opkssh](opkssh.md) - SSH with OpenPubkey -###### Auto generated by spf13/cobra on 20-Aug-2026 +###### Auto generated by spf13/cobra on 23-Jun-2026 diff --git a/main.go b/main.go index d7b83a80..ee0abc27 100644 --- a/main.go +++ b/main.go @@ -209,7 +209,7 @@ Arguments: // Define flags for login. loginCmd.Flags().BoolVar(&autoRefreshArg, "auto-refresh", false, "Automatically refresh PK token after login") - loginCmd.Flags().StringVar(&configPathArg, "config-path", "", "Path to the client config file. Default: the first existing of $XDG_CONFIG_HOME/opk/config.yml (~/.config/opk/config.yml on linux/macOS, %AppData%\\opk\\config.yml on windows) and the legacy ~/.opk/config.yml") + loginCmd.Flags().StringVar(&configPathArg, "config-path", "", config.ConfigPathFlagHelp) loginCmd.Flags().BoolVar(&createConfigArg, "create-config", false, "Creates a client config file if it does not exist") loginCmd.Flags().BoolVar(&configureArg, "configure", false, "Apply changes to ssh config and create ~/.ssh/opkssh directory") loginCmd.Flags().StringVar(&logDirArg, "log-dir", "", "Directory to write output logs") @@ -482,7 +482,7 @@ Exit code: 0 if all entries are valid, 1 if any warnings or errors are found.`, }, } - providerListCmd.Flags().StringVar(&configPathArg, "config-path", "", "Path to the client config file. Default: the first existing of $XDG_CONFIG_HOME/opk/config.yml (~/.config/opk/config.yml on linux/macOS, %AppData%\\opk\\config.yml on windows) and the legacy ~/.opk/config.yml.") + providerListCmd.Flags().StringVar(&configPathArg, "config-path", "", config.ConfigPathFlagHelp) providerCmd.AddCommand(providerListCmd) From 18e04ff02bc090680e0e989a255146dbdb6c6a50 Mon Sep 17 00:00:00 2001 From: Jason Bowman Date: Thu, 20 Aug 2026 18:51:13 -0700 Subject: [PATCH 09/13] fix: quote IdentityFile fragment paths containing blanks OpenSSH mis-parses an unquoted ssh_config path with a space (common with Windows home directories). identityFileLine now double-quotes a path containing blanks, per ssh_config token rules; since login's writer and logout's remover both render lines through this one function, matching and removal stay consistent by construction. Paths without blanks stay unquoted so fragment entries written by earlier versions keep matching exactly. --- commands/login.go | 8 +++++++- commands/login_test.go | 29 +++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/commands/login.go b/commands/login.go index 3df01ae1..1adbbf22 100644 --- a/commands/login.go +++ b/commands/login.go @@ -1167,8 +1167,14 @@ func classifySlot(fs afero.Fs, privKeyPath, pubKeyPath string, identity keyIdent // identityFileLine renders a config-fragment entry for a private key path. // The format is a contract between login (which writes entries) and logout -// (which removes them); both sides must go through this function. +// (which removes them); both sides must go through this function. A path +// containing blanks is double-quoted per ssh_config token rules — OpenSSH +// mis-parses an unquoted path with a space. Paths without blanks stay +// unquoted so entries written by earlier versions keep matching exactly. func identityFileLine(privKeyPath string) string { + if strings.ContainsAny(privKeyPath, " \t") { + return `IdentityFile "` + privKeyPath + `"` + } return "IdentityFile " + privKeyPath } diff --git a/commands/login_test.go b/commands/login_test.go index f2ffe24e..64bf15fb 100644 --- a/commands/login_test.go +++ b/commands/login_test.go @@ -1717,3 +1717,32 @@ func TestOpkSSHDirForeignTaggedPathErrors(t *testing.T) { _, err := cmd.writeKeysToOpkSSHDir(pem, certBytes, identity) require.ErrorContains(t, err, "holds a different identity") } + +func TestIdentityFileLineQuoting(t *testing.T) { + require.Equal(t, "IdentityFile /home/user/.ssh/opkssh/key", identityFileLine("/home/user/.ssh/opkssh/key")) + require.Equal(t, `IdentityFile "/home/John Smith/.ssh/opkssh/key"`, identityFileLine("/home/John Smith/.ssh/opkssh/key")) + + // Round-trip the writer/remover contract under a blank-bearing home + // directory (common on Windows, legal everywhere). + t.Setenv("HOME", "/home/John Smith") + cmd, afs, dirPath, identity, pem, certBytes := opkDirFixture(t) + + written, err := cmd.writeKeysToOpkSSHDir(pem, certBytes, identity) + require.NoError(t, err) + require.Contains(t, written, " ", "test premise: the key path must contain a blank") + configPath := filepath.Join(dirPath, "config") + quotedLine := identityFileLine(written) + require.Contains(t, strings.Split(string(mustReadFile(t, afs, configPath)), "\n"), quotedLine, + "a blank-bearing path must be written quoted") + + // A re-login must dedup against the quoted form, not duplicate it. + written2, err := cmd.writeKeysToOpkSSHDir(pem, certBytes, identity) + require.NoError(t, err) + require.Equal(t, written, written2) + require.Equal(t, 1, strings.Count(string(mustReadFile(t, afs, configPath)), quotedLine)) + + // Logout's remover must strip exactly this entry. + lo := &LogoutCmd{Fs: cmd.Fs} + require.NoError(t, lo.removeFromOpkSSHConfig(configPath, written)) + require.NotContains(t, string(mustReadFile(t, afs, configPath)), quotedLine) +} From c9885e3109f4c3e6c0e92cdc6113a936aefe5016 Mon Sep 17 00:00:00 2001 From: Jason Bowman Date: Thu, 20 Aug 2026 18:54:39 -0700 Subject: [PATCH 10/13] fix: skip the blank-home fragment round-trip on Windows t.Setenv(HOME) does not drive os.UserHomeDir on Windows (it reads USERPROFILE), so the round-trip's blank-bearing home premise cannot hold there; the identityFileLine rendering assertions still run on every platform. Same guard pattern as TestResolveClientConfigPath. --- commands/login_test.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/commands/login_test.go b/commands/login_test.go index 64bf15fb..76a13e1b 100644 --- a/commands/login_test.go +++ b/commands/login_test.go @@ -1722,6 +1722,13 @@ func TestIdentityFileLineQuoting(t *testing.T) { require.Equal(t, "IdentityFile /home/user/.ssh/opkssh/key", identityFileLine("/home/user/.ssh/opkssh/key")) require.Equal(t, `IdentityFile "/home/John Smith/.ssh/opkssh/key"`, identityFileLine("/home/John Smith/.ssh/opkssh/key")) + if runtime.GOOS == "windows" { + // The round-trip below pins a blank-bearing home via HOME, which does + // not drive os.UserHomeDir on Windows. The rendering assertions above + // already ran. + t.Skip("home directory is not resolved via HOME on Windows") + } + // Round-trip the writer/remover contract under a blank-bearing home // directory (common on Windows, legal everywhere). t.Setenv("HOME", "/home/John Smith") From d28afba8b75bb825339cb2f6d9e51f80aa16f4f9 Mon Sep 17 00:00:00 2001 From: Jason Bowman Date: Thu, 20 Aug 2026 19:00:07 -0700 Subject: [PATCH 11/13] docs: condense comments to their load-bearing constraints, ascii-only --- README.md | 2 +- commands/config/client_config.go | 9 +-- commands/config/config_path_unix.go | 8 +- commands/login.go | 121 +++++++++++----------------- commands/login_test.go | 13 ++- commands/logout.go | 3 +- 6 files changed, 60 insertions(+), 96 deletions(-) diff --git a/README.md b/README.md index f6ff6475..f2040ac2 100644 --- a/README.md +++ b/README.md @@ -107,7 +107,7 @@ opkssh login This opens a browser window to select which OpenID Provider you want to authenticate against. After successfully authenticating opkssh generates an SSH public key in `~/.ssh/id_ecdsa` which contains your PK Token. -If the default key files already hold a different identity — for instance when you log in with a second account or provider — the new keys are written to the opkssh identity directory (`~/.ssh/opkssh/`) instead of overwriting them. +If the default key files already hold a different identity, for instance when you log in with a second account or provider, the new keys are written to the opkssh identity directory (`~/.ssh/opkssh/`) instead of overwriting them. By default this ssh key expires after 24 hours and you must run `opkssh login` to generate a new ssh key. Since your PK Token has been saved as an SSH key you can SSH as normal: diff --git a/commands/config/client_config.go b/commands/config/client_config.go index a11648dd..ad297028 100644 --- a/commands/config/client_config.go +++ b/commands/config/client_config.go @@ -91,9 +91,8 @@ func clientConfigCandidatePaths() ([]string, error) { if homeDir, err := os.UserHomeDir(); err == nil { candidates = append(candidates, filepath.Join(homeDir, ".opk", "config.yml")) } else { - // Never drop the legacy candidate silently: a user with only a - // legacy config would otherwise get an unexplained fresh-config - // resolution. + // Never drop the legacy candidate silently: a user whose only config + // is the legacy one would get an unexplained fresh-config resolution. log.Printf("warning: could not determine home directory, ignoring legacy ~/.opk/config.yml: %v", err) } if len(candidates) == 0 { @@ -104,8 +103,8 @@ func clientConfigCandidatePaths() ([]string, error) { // ResolveClientConfigPath resolves the client config path and reports // whether a config file exists there. An explicitly provided path is used -// as-is. Otherwise the first existing candidate wins — so a legacy -// ~/.opk/config.yml keeps working untouched — and when no config exists +// as-is. Otherwise the first existing candidate wins (so a legacy +// ~/.opk/config.yml keeps working untouched), and when no config exists // anywhere the path falls to the first candidate, the XDG-preferred // location, which is where a new config is then created. func ResolveClientConfigPath(fs afero.Fs, configPath *string) (bool, error) { diff --git a/commands/config/config_path_unix.go b/commands/config/config_path_unix.go index 3a8ea20c..c5d256d2 100644 --- a/commands/config/config_path_unix.go +++ b/commands/config/config_path_unix.go @@ -24,11 +24,9 @@ import ( "path/filepath" ) -// userConfigDir returns the platform's user config directory used when -// $XDG_CONFIG_HOME is not set. This is deliberately ~/.config on every -// Unix-like system, macOS included: opkssh is a CLI tool, and CLI convention -// (and the XDG default) is ~/.config rather than ~/Library/Application -// Support. +// userConfigDir is the fallback when $XDG_CONFIG_HOME is unset: deliberately +// ~/.config on every Unix-like system, macOS included. CLI convention (and +// the XDG default), not ~/Library/Application Support. func userConfigDir() (string, error) { homeDir, err := os.UserHomeDir() if err != nil { diff --git a/commands/login.go b/commands/login.go index 1adbbf22..337045e3 100644 --- a/commands/login.go +++ b/commands/login.go @@ -184,10 +184,8 @@ func (l *LoginCmd) Run(ctx context.Context) error { return err } else { l.Config = client_config - // Multiple config locations are possible (see - // config.ResolveClientConfigPath); naming the winner - // defuses silent shadowing, e.g. a hand-created legacy file - // losing to an existing XDG-location config. + // Naming the winner defuses silent shadowing, e.g. a + // hand-created legacy file losing to an XDG-location config. if l.Verbosity >= 1 { log.Printf("using client config at %s", l.ConfigPathArg) } @@ -578,8 +576,8 @@ func (l *LoginCmd) login(ctx context.Context, provider providers.OpenIdProvider, } // Write ssh secret key and public key to filesystem. fallbackKeyPath is - // set when the default key slots were all taken by other identities and - // the keys landed in the opkssh identity directory instead. + // set when the keys landed in the opkssh identity directory instead of a + // default slot. var fallbackKeyPath string if l.PrintKeyArg { w := l.out() @@ -592,10 +590,8 @@ func (l *LoginCmd) login(ctx context.Context, provider providers.OpenIdProvider, // Best-effort: addCertToAgent warns and returns rather than failing the login. if !l.PrintKeyArg { addedToAgent := l.addCertToAgent(certBytes, signer) - // The remediation warning is decided after the agent attempt: keys - // written to the fallback location are not tried by ssh on their - // own, so the user must hear about it exactly when no agent ended up - // holding the key either. + // Warn only when the fallback was used AND no agent holds the key: + // ssh does not try non-default file names on its own. if fallbackKeyPath != "" && !addedToAgent { fmt.Fprintf(l.out(), "warning: ssh will not try %s automatically and no ssh-agent holds it; add an IdentityFile entry for it, use -i, or run `opkssh login --configure`\n", fallbackKeyPath) } @@ -927,11 +923,9 @@ func (l *LoginCmd) writeKeysToOpkSSHDir(secKeyPem []byte, certBytes []byte, iden opkSshUserPath := filepath.Join(userhomeDir, opkSshPath) opkSshConfigPath := filepath.Join(opkSshUserPath, configFileName) - // The directory and its config fragment are created on demand: this - // writer also serves logins whose default key slots are all taken, which - // may happen before --configure has ever run. Writing the IdentityFile - // line is inert until ~/.ssh/config gains the Include directive, but it - // makes a later --configure pick every key up instantly. + // Created on demand: this writer also serves overflow logins before + // --configure has ever run. The IdentityFile line is inert without the + // Include in ~/.ssh/config, but a later --configure adopts every key. if err := l.Fs.MkdirAll(opkSshUserPath, 0o700); err != nil { return "", err } @@ -942,18 +936,13 @@ func (l *LoginCmd) writeKeysToOpkSSHDir(secKeyPem []byte, certBytes []byte, iden privKeyPath := filepath.Join(opkSshUserPath, sshKeyName) pubKeyPath := privKeyPath + "-cert.pub" - // A different account at the same provider maps to the same - // - file name; it must not be clobbered. Policy for - // this tier: a slot is writable only when it is fully free or the - // existing certificate provably belongs to the same identity; anything - // else (different identity, unparseable, or a private key without its - // certificate) disambiguates with an identity tag — and the same rule - // applies at the tagged path, erroring rather than silently replacing a - // foreign occupant. + // A different account at the same provider maps to the same file name and + // must not be clobbered; see dirSlotWritable. The same rule applies at + // the tagged path: a foreign occupant there is an error, never a silent + // overwrite. if !dirSlotWritable(classifySlot(l.Fs, privKeyPath, pubKeyPath, identity)) { - // Tagging a nothing would produce a stable but non-identifying file - // name; failing loudly is clearer (unreachable in practice — the PK - // token just completed a login). + // Tagging an invalid identity would hash nothing; fail loudly instead + // (unreachable in practice; the token just completed a login). if !identity.valid { return "", errors.New("cannot disambiguate the key file name: no identity could be extracted from the PK token") } @@ -973,10 +962,9 @@ func (l *LoginCmd) writeKeysToOpkSSHDir(secKeyPem []byte, certBytes []byte, iden return "", err } - // add key to config. Whole-line match (a tagged path contains its - // untagged prefix, so a substring check would wrongly skip needed - // entries); when the line is already present the fragment is left - // entirely untouched. + // Whole-line match: a tagged path contains its untagged prefix, so a + // substring check would wrongly skip entries. An already-present line + // means no rewrite. configContent, err := afs.ReadFile(opkSshConfigPath) if err != nil { if !os.IsNotExist(err) { @@ -1031,18 +1019,14 @@ func (l *LoginCmd) writeKeysToSSHDir(seckeySshPem []byte, certBytes []byte, iden pubkeyPath := seckeyPath + "-cert.pub" info := classifySlot(l.Fs, seckeyPath, pubkeyPath, identity) - // Policy for the default tier, preserving pre-change behavior - // exactly: an absent private key makes the slot writable even over - // an orphaned or foreign certificate. With the private key present, - // only a provably same-identity certificate may be overwritten — - // plus the legacy case of a certificate with no extractable PK - // token identity but the comment exactly "openpubkey", which was - // the pre-change reuse condition; this repo does not control the PK - // token wire format's history, and demoting a re-login to a - // relocated key on a parse failure would silently shadow the fresh - // key with the stale file. A parseable certificate for a different - // identity, an orphaned private key, and an unreadable certificate - // are protected. + // Default-tier policy: an absent private key makes the slot writable + // even over an orphaned or foreign certificate; with the key present, + // only a same-identity certificate or a legacy one (unparseable PK + // token, comment exactly "openpubkey") may be overwritten. The + // legacy case matters because this repo does not control the PK token + // wire format, and skipping on a parse failure would silently shadow + // a re-login's fresh key with its own stale file. All else is + // protected. if !info.privExists || info.cert == certSameIdentity || info.cert == certLegacyComment { return "", l.writeKeys(seckeyPath, pubkeyPath, seckeySshPem, certBytes) } @@ -1051,11 +1035,9 @@ func (l *LoginCmd) writeKeysToSSHDir(seckeySshPem []byte, certBytes []byte, iden } } - // Every default slot belongs to someone else: write a per-identity file - // into the opkssh identity directory instead of clobbering. ssh does not - // try non-default file names on its own — the key is reachable through - // ssh-agent (login loads it there when one is available), an - // IdentityFile entry, -i, or the --configure mechanism. + // Every default slot belongs to someone else: fall through to the opkssh + // identity directory. The key stays reachable via ssh-agent, an + // IdentityFile entry, -i, or --configure. return l.writeKeysToOpkSSHDir(seckeySshPem, certBytes, identity) } @@ -1068,10 +1050,9 @@ type keyIdentity struct { valid bool } -// sameAs reports whether two identities are provably the same. It fails -// closed by construction: an invalid identity — nil token, failed claim -// extraction, unparseable certificate — never compares equal to anything, -// because equality is what authorizes overwriting a key file. +// sameAs fails closed: an invalid identity (nil token, failed extraction, +// unparseable certificate) never compares equal; equality is what authorizes +// overwriting a key file. func (k keyIdentity) sameAs(other keyIdentity) bool { return k.valid && other.valid && k == other } @@ -1091,9 +1072,8 @@ func pktIdentity(pkt *pktoken.PKToken) keyIdentity { return keyIdentity{issuer: issuer, audience: audience, subject: subject, valid: true} } -// certFileIdentity extracts the identity from the bytes of a certificate -// file written by opkssh; invalid when the file does not hold an opkssh -// certificate with an extractable identity. +// certFileIdentity extracts the identity from a certificate file's bytes; +// invalid unless it holds an opkssh certificate with an extractable identity. func certFileIdentity(certBytes []byte) keyIdentity { pubkey, _, _, _, err := ssh.ParseAuthorizedKey(certBytes) if err != nil { @@ -1135,9 +1115,8 @@ type slotInfo struct { cert certState } -// classifySlot answers "who holds this slot?" for a private-key path and its -// -cert.pub companion. It is the mechanism shared by every key-writing tier; -// each tier maps the states to its own write policy. +// classifySlot answers "who holds this slot?", the mechanism shared by every +// key-writing tier; each tier maps the states to its own write policy. func classifySlot(fs afero.Fs, privKeyPath, pubKeyPath string, identity keyIdentity) slotInfo { info := slotInfo{privExists: fsFileExists(fs, privKeyPath)} if !fsFileExists(fs, pubKeyPath) { @@ -1165,12 +1144,10 @@ func classifySlot(fs afero.Fs, privKeyPath, pubKeyPath string, identity keyIdent return info } -// identityFileLine renders a config-fragment entry for a private key path. -// The format is a contract between login (which writes entries) and logout -// (which removes them); both sides must go through this function. A path -// containing blanks is double-quoted per ssh_config token rules — OpenSSH -// mis-parses an unquoted path with a space. Paths without blanks stay -// unquoted so entries written by earlier versions keep matching exactly. +// identityFileLine renders a fragment entry, the format contract between +// login (writer) and logout (remover); both must use this function. A +// blank-bearing path is double-quoted (unquoted, OpenSSH mis-parses it); +// blank-free paths stay unquoted so existing entries keep matching. func identityFileLine(privKeyPath string) string { if strings.ContainsAny(privKeyPath, " \t") { return `IdentityFile "` + privKeyPath + `"` @@ -1178,15 +1155,12 @@ func identityFileLine(privKeyPath string) string { return "IdentityFile " + privKeyPath } -// fragmentLines splits config-fragment content into lines, tolerating both -// \n and \r\n endings. +// fragmentLines splits fragment content into lines, tolerating \r\n. func fragmentLines(fragment []byte) []string { return strings.Split(strings.ReplaceAll(string(fragment), "\r\n", "\n"), "\n") } -// commentField preserves the historical key-comment format, which printed -// "unknown" for a claim that could not be extracted rather than an empty -// field. +// commentField keeps the historical "unknown" placeholder in key comments. func commentField(s string) string { if s == "" { return "unknown" @@ -1194,11 +1168,9 @@ func commentField(s string) string { return s } -// identityTag returns a short, stable tag for an identity, used to -// disambiguate key file names for different accounts at the same provider. -// The full triple is hashed — not the subject alone — so that two client IDs -// sharing a truncated prefix in the file name cannot alias, and because some -// OPs use email addresses as subjects, which do not belong in file names. +// identityTag disambiguates key file names for different accounts at the +// same provider. The full triple is hashed: truncated client-ID prefixes +// cannot alias, and subjects (sometimes emails) stay out of file names. func identityTag(identity keyIdentity) string { digest := sha256.Sum256([]byte(identity.issuer + "|" + identity.audience + "|" + identity.subject)) return hex.EncodeToString(digest[:4]) @@ -1261,8 +1233,7 @@ func (l *LoginCmd) makeSSHKeyFileName(pkt *pktoken.PKToken) string { } func fsFileExists(fs afero.Fs, fPath string) bool { - // Stat, not Open: the previous Open-based check leaked a file descriptor - // per call. + // Stat, not Open: an Open-based check leaks a file descriptor per call. _, err := fs.Stat(fPath) return !errors.Is(err, os.ErrNotExist) } diff --git a/commands/login_test.go b/commands/login_test.go index 76a13e1b..e74d18f7 100644 --- a/commands/login_test.go +++ b/commands/login_test.go @@ -870,9 +870,8 @@ func TestWriteKeysToDestination(t *testing.T) { wantErrContains: "failed to write SSH keys to filesystem", }, { - // A missing opkssh dir or config fragment is no longer an error - // (both are created on demand), so the write failure is induced - // with a read-only filesystem instead. + // A missing opkssh dir or fragment is created on demand, so the + // write failure is induced with a read-only filesystem. name: "opkssh dir write failure is wrapped", sshConfigured: true, readOnly: true, @@ -1575,10 +1574,8 @@ func TestWriteKeysIdentitySlots(t *testing.T) { } func TestWriteKeysLegacyCommentFallback(t *testing.T) { - // A cert written by an older opkssh whose embedded PK token no longer - // parses must still be overwritten in place when its comment is exactly - // "openpubkey" — a single-identity re-login must never be demoted to a - // relocated key by a parse failure. + // A legacy cert with an unparseable PK token but comment "openpubkey" + // must be overwritten in place: never demote a re-login on parse failure. pkt, signer, _ := Mocks(t, ECDSA) certBytes, pem, err := createSSHCert(pkt, signer, []string{"test"}) require.NoError(t, err) @@ -1685,7 +1682,7 @@ func TestOpkSSHDirPartialStateDisambiguates(t *testing.T) { func TestFragmentWholeLineDedup(t *testing.T) { // The discriminating case for whole-line matching: the fragment already // holds the TAGGED path's line, whose text contains the base path as a - // prefix. A base-path write must still add its own line — a substring + // prefix. A base-path write must still add its own line; a substring // check would wrongly skip it. cmd, afs, dirPath, identity, pem, certBytes := opkDirFixture(t) diff --git a/commands/logout.go b/commands/logout.go index befe5559..9270ef7a 100644 --- a/commands/logout.go +++ b/commands/logout.go @@ -289,8 +289,7 @@ func (l *LogoutCmd) removeFromOpkSSHConfig(configPath string, seckeyPath string) return nil // Config file doesn't exist, nothing to clean up } - // identityFileLine and fragmentLines are the shared fragment-format - // contract with login's writer — both sides must stay on these helpers. + // Shared fragment-format contract with login's writer; stay on these helpers. identityLine := identityFileLine(seckeyPath) lines := fragmentLines(content) var newLines []string From e1b44b2a8ac901b5650f92bd577944cff0238988 Mon Sep 17 00:00:00 2001 From: Jason Bowman Date: Thu, 20 Aug 2026 19:00:58 -0700 Subject: [PATCH 12/13] docs: clarify two condensed comments --- commands/config/config_path_unix.go | 6 +++--- commands/login.go | 5 +++-- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/commands/config/config_path_unix.go b/commands/config/config_path_unix.go index c5d256d2..0136980c 100644 --- a/commands/config/config_path_unix.go +++ b/commands/config/config_path_unix.go @@ -24,9 +24,9 @@ import ( "path/filepath" ) -// userConfigDir is the fallback when $XDG_CONFIG_HOME is unset: deliberately -// ~/.config on every Unix-like system, macOS included. CLI convention (and -// the XDG default), not ~/Library/Application Support. +// userConfigDir is the fallback when $XDG_CONFIG_HOME is unset. It is +// deliberately ~/.config on every Unix-like system, macOS included, because +// CLI tools follow the XDG default, not ~/Library/Application Support. func userConfigDir() (string, error) { homeDir, err := os.UserHomeDir() if err != nil { diff --git a/commands/login.go b/commands/login.go index 337045e3..6769f247 100644 --- a/commands/login.go +++ b/commands/login.go @@ -184,8 +184,9 @@ func (l *LoginCmd) Run(ctx context.Context) error { return err } else { l.Config = client_config - // Naming the winner defuses silent shadowing, e.g. a - // hand-created legacy file losing to an XDG-location config. + // Log which config file won the resolution. Without this, a + // user who edits the legacy file while an XDG config exists + // sees their edits silently ignored. if l.Verbosity >= 1 { log.Printf("using client config at %s", l.ConfigPathArg) } From 4bd8391a7e0486d4c1537aee3e9b0f1fe7a6b732 Mon Sep 17 00:00:00 2001 From: Jason Bowman Date: Thu, 20 Aug 2026 19:04:16 -0700 Subject: [PATCH 13/13] docs: anchor comment terminology Every shorthand term is now defined where it lives: the config fragment is named as ~/.ssh/opkssh/config with its Include relationship at identityFileLine, key-file slots are defined at certState, and the two key-writing destinations are named instead of called tiers. Subjectless and provenance sentences are rewritten to name their objects. --- commands/login.go | 44 ++++++++++++++++++++++++++---------------- commands/login_test.go | 20 ++++++++++--------- commands/logout.go | 3 ++- 3 files changed, 40 insertions(+), 27 deletions(-) diff --git a/commands/login.go b/commands/login.go index 6769f247..6f747986 100644 --- a/commands/login.go +++ b/commands/login.go @@ -924,9 +924,11 @@ func (l *LoginCmd) writeKeysToOpkSSHDir(secKeyPem []byte, certBytes []byte, iden opkSshUserPath := filepath.Join(userhomeDir, opkSshPath) opkSshConfigPath := filepath.Join(opkSshUserPath, configFileName) - // Created on demand: this writer also serves overflow logins before - // --configure has ever run. The IdentityFile line is inert without the - // Include in ~/.ssh/config, but a later --configure adopts every key. + // The directory and its config fragment are created on demand, because + // this writer also serves logins whose default key slots are all taken, + // which can happen before --configure has ever run. The IdentityFile + // line is inert without the Include in ~/.ssh/config, but a later + // --configure adopts every key already listed. if err := l.Fs.MkdirAll(opkSshUserPath, 0o700); err != nil { return "", err } @@ -1020,8 +1022,9 @@ func (l *LoginCmd) writeKeysToSSHDir(seckeySshPem []byte, certBytes []byte, iden pubkeyPath := seckeyPath + "-cert.pub" info := classifySlot(l.Fs, seckeyPath, pubkeyPath, identity) - // Default-tier policy: an absent private key makes the slot writable - // even over an orphaned or foreign certificate; with the key present, + // Policy for the default ~/.ssh key names: an absent private key + // makes the slot writable even over an orphaned or foreign + // certificate; with the key present, // only a same-identity certificate or a legacy one (unparseable PK // token, comment exactly "openpubkey") may be overwritten. The // legacy case matters because this repo does not control the PK token @@ -1037,8 +1040,8 @@ func (l *LoginCmd) writeKeysToSSHDir(seckeySshPem []byte, certBytes []byte, iden } // Every default slot belongs to someone else: fall through to the opkssh - // identity directory. The key stays reachable via ssh-agent, an - // IdentityFile entry, -i, or --configure. + // identity directory (~/.ssh/opkssh). The key stays reachable via + // ssh-agent, an IdentityFile entry, -i, or --configure. return l.writeKeysToOpkSSHDir(seckeySshPem, certBytes, identity) } @@ -1097,8 +1100,8 @@ func certKeyIdentity(pubkey ssh.PublicKey) keyIdentity { return pktIdentity(pkt) } -// certState classifies the -cert.pub side of a key-file slot relative to the -// identity now logging in. +// certState classifies the -cert.pub side of a key-file slot (a private key +// path plus its -cert.pub companion) relative to the identity now logging in. type certState int const ( @@ -1116,8 +1119,9 @@ type slotInfo struct { cert certState } -// classifySlot answers "who holds this slot?", the mechanism shared by every -// key-writing tier; each tier maps the states to its own write policy. +// classifySlot answers "who holds this slot?". It is shared by both +// key-writing destinations, the default ~/.ssh key names and the opkssh +// identity directory; each maps the states to its own write policy. func classifySlot(fs afero.Fs, privKeyPath, pubKeyPath string, identity keyIdentity) slotInfo { info := slotInfo{privExists: fsFileExists(fs, privKeyPath)} if !fsFileExists(fs, pubKeyPath) { @@ -1145,10 +1149,14 @@ func classifySlot(fs afero.Fs, privKeyPath, pubKeyPath string, identity keyIdent return info } -// identityFileLine renders a fragment entry, the format contract between -// login (writer) and logout (remover); both must use this function. A -// blank-bearing path is double-quoted (unquoted, OpenSSH mis-parses it); -// blank-free paths stay unquoted so existing entries keep matching. +// identityFileLine renders one IdentityFile entry for the opkssh config +// fragment: the file ~/.ssh/opkssh/config, which holds one IdentityFile line +// per written key and which ~/.ssh/config Includes once --configure has run. +// The entry format is a contract between login (which writes entries) and +// logout (which removes them); both must use this function. A path containing +// blanks is double-quoted, because OpenSSH mis-parses an unquoted path with a +// space; paths without blanks stay unquoted so entries written by earlier +// versions keep matching. func identityFileLine(privKeyPath string) string { if strings.ContainsAny(privKeyPath, " \t") { return `IdentityFile "` + privKeyPath + `"` @@ -1156,12 +1164,14 @@ func identityFileLine(privKeyPath string) string { return "IdentityFile " + privKeyPath } -// fragmentLines splits fragment content into lines, tolerating \r\n. +// fragmentLines splits the config fragment's content into lines, tolerating +// both \n and \r\n endings. func fragmentLines(fragment []byte) []string { return strings.Split(strings.ReplaceAll(string(fragment), "\r\n", "\n"), "\n") } -// commentField keeps the historical "unknown" placeholder in key comments. +// commentField substitutes "unknown" for a claim that could not be +// extracted, preserving the historical format of the key-file comment. func commentField(s string) string { if s == "" { return "unknown" diff --git a/commands/login_test.go b/commands/login_test.go index e74d18f7..1a19ed40 100644 --- a/commands/login_test.go +++ b/commands/login_test.go @@ -870,8 +870,9 @@ func TestWriteKeysToDestination(t *testing.T) { wantErrContains: "failed to write SSH keys to filesystem", }, { - // A missing opkssh dir or fragment is created on demand, so the - // write failure is induced with a read-only filesystem. + // writeKeysToOpkSSHDir creates a missing ~/.ssh/opkssh directory + // and config fragment on demand, so the write failure is induced + // with a read-only filesystem. name: "opkssh dir write failure is wrapped", sshConfigured: true, readOnly: true, @@ -1527,7 +1528,8 @@ func TestWriteKeysIdentitySlots(t *testing.T) { require.NoError(t, afs.WriteFile(skKeyPath, []byte("foreign"), 0o600)) require.NoError(t, afs.WriteFile(skKeyPath+"-cert.pub", foreignPubkeyLine(t), 0o644)) - // First login writes the default slot, exactly as before this change. + // A first login writes the default ~/.ssh/id_ecdsa slot (the unchanged + // single-identity behavior). fallback, err := newCmd(pktA).writeKeysToDestination("", pemA, certA) require.NoError(t, err) require.Empty(t, fallback) @@ -1544,7 +1546,7 @@ func TestWriteKeysIdentitySlots(t *testing.T) { require.False(t, dirExists, "opkssh dir must not appear while a single identity is in play") // Different identity: the occupied slots are preserved and the keys land - // in the opkssh identity directory, with a fragment entry. + // in the opkssh identity directory, with a config-fragment entry. fallbackB, err := newCmd(pktB).writeKeysToDestination("", pemB, certB) require.NoError(t, err) require.NotEmpty(t, fallbackB) @@ -1558,7 +1560,7 @@ func TestWriteKeysIdentitySlots(t *testing.T) { require.Contains(t, configLines, identityFileLine(fallbackB)) // Second identity re-login: same fallback file overwritten in place, no - // duplicate fragment line. + // duplicate config-fragment line. fallbackB2, err := newCmd(pktB).writeKeysToDestination("", pemB2, certB2) require.NoError(t, err) require.Equal(t, fallbackB, fallbackB2) @@ -1680,10 +1682,10 @@ func TestOpkSSHDirPartialStateDisambiguates(t *testing.T) { } func TestFragmentWholeLineDedup(t *testing.T) { - // The discriminating case for whole-line matching: the fragment already - // holds the TAGGED path's line, whose text contains the base path as a - // prefix. A base-path write must still add its own line; a substring - // check would wrongly skip it. + // The discriminating case for whole-line matching: the config fragment + // (~/.ssh/opkssh/config) already holds the TAGGED path's IdentityFile + // line, whose text contains the base path as a prefix. A base-path write + // must still add its own line; a substring check would wrongly skip it. cmd, afs, dirPath, identity, pem, certBytes := opkDirFixture(t) basePath := filepath.Join(dirPath, cmd.makeSSHKeyFileName(cmd.pkt)) diff --git a/commands/logout.go b/commands/logout.go index 9270ef7a..6df2f669 100644 --- a/commands/logout.go +++ b/commands/logout.go @@ -289,7 +289,8 @@ func (l *LogoutCmd) removeFromOpkSSHConfig(configPath string, seckeyPath string) return nil // Config file doesn't exist, nothing to clean up } - // Shared fragment-format contract with login's writer; stay on these helpers. + // Render and split entries via login.go's identityFileLine/fragmentLines + // so this remover cannot drift from the writer's entry format. identityLine := identityFileLine(seckeyPath) lines := fragmentLines(content) var newLines []string