diff --git a/README.md b/README.md index f512cefc..f2040ac2 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 e27e03c1..ad297028 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) { @@ -59,21 +60,78 @@ 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) +// 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 +// 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 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 { + return nil, fmt.Errorf("failed to determine the user config directory: %w", platformDirErr) + } + return candidates, nil +} + +// 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) (bool, error) { + afs := &afero.Afero{Fs: fs} + if *configPath != "" { + found, err := afs.Exists(*configPath) + return err == nil && found, nil + } + candidates, err := clientConfigCandidatePaths() + if err != nil { + return false, err + } + for _, candidate := range candidates { + if exists, err := afs.Exists(candidate); err == nil && exists { + *configPath = candidate + return true, nil } - *configPath = filepath.Join(dir, ".opk", "config.yml") } - return nil + *configPath = candidates[0] + 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(&configPath); err != nil { + if _, err := ResolveClientConfigPath(Fs, &configPath); err != nil { return nil, err } @@ -93,10 +151,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 04c235b9..1bbaab7a 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" ) @@ -76,3 +79,141 @@ providers: require.NotNil(t, clientConfig) require.Equal(t, clientConfig.Providers[0].SendAccessToken, true) } + +func TestParseConfigWithAgentLifetime(t *testing.T) { + // 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` + + clientConfig, err := NewClientConfig([]byte(c)) + require.NoError(t, err) + require.NotNil(t, clientConfig) + 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 + wantFound bool + }{ + { + 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, + 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 + wantFound: false, + }, + { + 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, + wantFound: true, + }, + { + 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, + wantFound: true, + }, + { + 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, + wantFound: false, + }, + } + + 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 + found, err := ResolveClientConfigPath(fs, &configPath) + require.NoError(t, err) + require.Equal(t, tt.expected, configPath) + require.Equal(t, tt.wantFound, found) + }) + } +} + +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..0136980c --- /dev/null +++ b/commands/config/config_path_unix.go @@ -0,0 +1,36 @@ +//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 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 { + 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/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..6f747986 100644 --- a/commands/login.go +++ b/commands/login.go @@ -21,18 +21,23 @@ import ( "context" "crypto" "crypto/ecdsa" + "crypto/sha256" "encoding/base64" + "encoding/hex" "encoding/json" "encoding/pem" "errors" "fmt" "io" "log" + "math" + "net" "net/url" "os" "path/filepath" "regexp" "slices" + "strconv" "strings" "time" @@ -49,6 +54,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 +106,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 +126,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 +146,7 @@ func NewLogin(autoRefreshArg bool, configPathArg string, createConfigArg bool, c KeyTypeArg: keyTypeArg, RemoteRedirectURI: remoteRedirectUri, PrincipalsArg: principalsArg, + AgentLifetimeArg: agentLifetimeArg, } } @@ -163,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.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) } @@ -175,6 +184,12 @@ func (l *LoginCmd) Run(ctx context.Context) error { return err } else { l.Config = client_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) + } } } else { if l.CreateConfigArg { @@ -186,6 +201,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) + } } } @@ -199,6 +217,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-based OIDC flow starts rather than 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 { @@ -551,15 +576,28 @@ 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 keys landed in the opkssh identity directory instead of a + // default slot. + 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 } + // Best-effort: addCertToAgent warns and returns rather than failing the login. + if !l.PrintKeyArg { + addedToAgent := l.addCertToAgent(certBytes, signer) + // 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) + } + } + if printIdToken { idTokenStr, err := PrettyIdToken(*pkt) if err != nil { @@ -646,7 +684,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 } @@ -675,6 +713,123 @@ func (l *LoginCmd) out() io.Writer { return os.Stdout } +// 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, 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 false + } + + lifetimeSecs, err := l.resolveAgentLifetimeSecs() + if err != nil { + fmt.Fprintf(l.out(), "warning: not adding certificate to ssh-agent: %v\n", err) + 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 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 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 false + } + defer conn.Close() + + // 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 false + } + + 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 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 +// --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 { + 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) + } + // 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) + } + 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 { + // 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) + } + 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) } @@ -731,29 +886,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) error { +func (l *LoginCmd) writeKeysToOpkSSHDir(secKeyPem []byte, certBytes []byte, identity keyIdentity) (string, error) { const ( opkSshPath = ".ssh/opkssh" @@ -762,107 +918,273 @@ 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, 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 + } - privKeyPath := filepath.Join(opkSshUserPath, sshKeyName) - pubKeyPath := filepath.Join(privKeyPath + "-cert.pub") + afs := &afero.Afero{Fs: l.Fs} - // 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 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 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") + } + sshKeyName = sshKeyName + "-" + identityTag(identity) + privKeyPath = filepath.Join(opkSshUserPath, sshKeyName) + pubKeyPath = privKeyPath + "-cert.pub" + 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) + } } - 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} + // 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 { - return fmt.Errorf("failed to read opk ssh config file (%s): %w", opkSshConfigPath, err) - } - - if !strings.Contains(string(configContent), privKeyPath) { - configContent = slices.Concat( - []byte("IdentityFile "+privKeyPath+"\n"), - configContent, - ) + if !os.IsNotExist(err) { + return "", fmt.Errorf("failed to read opk ssh config file (%s): %w", opkSshConfigPath, err) + } + configContent = nil + } + 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) + 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 +} - err = afs.WriteFile(opkSshConfigPath, configContent, 0600) - if err != nil { - return fmt.Errorf("failed to write opk ssh config file (%s): %w", opkSshConfigPath, err) +// 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 } - - // write ssh key files - return l.writeKeysComment(privKeyPath, pubKeyPath, secKeyPem, certBytes, comment) + 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 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. 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()) } for _, keyFilename := range keyFileNames { seckeyPath := filepath.Join(sshPath, keyFilename) pubkeyPath := seckeyPath + "-cert.pub" + info := classifySlot(l.Fs, seckeyPath, pubkeyPath, identity) + + // 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 + // 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) + } + if info.cert == certUnreadable { + log.Println("Failed to read or parse:", pubkeyPath) + } + } - 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 { - // 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) - if err != nil { - log.Println("Failed to parse:", pubkeyPath) - continue - } + // Every default slot belongs to someone else: fall through to the opkssh + // identity directory (~/.ssh/opkssh). The key stays reachable via + // ssh-agent, an IdentityFile entry, -i, or --configure. + return l.writeKeysToOpkSSHDir(seckeySshPem, certBytes, identity) +} - // If the key comment is "openpubkey" then we generated it - if comment == "openpubkey" { - return l.writeKeys(seckeyPath, pubkeyPath, seckeySshPem, certBytes) - } - } +// 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 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 +} + +// 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{} + } + issuer, errIss := pkt.Issuer() + audience, errAud := pkt.Audience() + subject, errSub := pkt.Subject() + if errIss != nil || errAud != nil || errSub != nil { + return keyIdentity{} + } + return keyIdentity{issuer: issuer, audience: audience, subject: subject, valid: true} +} + +// 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 { + return keyIdentity{} } - return fmt.Errorf("no default ssh key file free for openpubkey") + return certKeyIdentity(pubkey) +} + +// certKeyIdentity extracts the identity from an already-parsed public key. +func certKeyIdentity(pubkey ssh.PublicKey) keyIdentity { + cert, isCert := pubkey.(*ssh.Certificate) + if !isCert { + return keyIdentity{} + } + smuggler := &sshcert.SshCertSmuggler{SshCert: cert} + pkt, err := smuggler.GetPKToken() + if err != nil { + return keyIdentity{} + } + return pktIdentity(pkt) +} + +// 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 ( + 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?". 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) { + 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 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 + `"` + } + return "IdentityFile " + privKeyPath +} + +// 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 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" + } + return s +} + +// 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]) } func (l *LoginCmd) writeKeys(seckeyPath string, pubkeyPath string, seckeySshPem []byte, certBytes []byte) error { @@ -921,8 +1243,9 @@ func (l *LoginCmd) makeSSHKeyFileName(pkt *pktoken.PKToken) string { return keyName } -func (l *LoginCmd) fileExists(fPath string) bool { - _, err := l.Fs.Open(fPath) +func fsFileExists(fs afero.Fs, fPath string) bool { + // Stat, not Open: an Open-based check leaks a file descriptor per call. + _, err := fs.Stat(fPath) return !errors.Is(err, os.ErrNotExist) } @@ -936,7 +1259,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 b37a97a2..1a19ed40 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" @@ -77,6 +79,11 @@ 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 + // that needs an agent points SSH_AUTH_SOCK at its own test agent after + // calling Mocks. + t.Setenv("SSH_AUTH_SOCK", "") + providerOpts := providers.DefaultMockProviderOpts() op, _, idtTemplate, err := providers.NewMockProvider(providerOpts) require.NoError(t, err) @@ -513,10 +520,160 @@ 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 at least 1 second", + }, + { + name: "negative flag value is an error", + cmd: LoginCmd{AgentLifetimeArg: "-5m"}, + errMsg: "must be at least 1 second", + }, + { + name: "sub-second flag value is an error", + cmd: LoginCmd{AgentLifetimeArg: "500ms"}, + errMsg: "must be at least 1 second", + }, + { + // 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", + }, + { + 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", + 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) +} + +// 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") + } + sockPath := filepath.Join(t.TempDir(), "agent.sock") + listener, err := net.Listen("unix", sockPath) + require.NoError(t, err) + t.Cleanup(func() { _ = listener.Close() }) + go func() { + conn, err := listener.Accept() + if err != nil { + return + } + _ = 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{} + l := &LoginCmd{AgentLifetimeArg: "2h", OutWriter: out} + require.True(t, 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) { @@ -713,9 +870,12 @@ func TestWriteKeysToDestination(t *testing.T) { wantErrContains: "failed to write SSH keys to 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: false, // SSHConfigured but no config file -> read fails + readOnly: true, wantErrContains: "failed to write SSH keys to OPK SSH dir", }, { @@ -748,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) @@ -1270,3 +1430,325 @@ 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", 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 +// 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) +} + +// 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 := 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 := 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) + 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{}} + } + + // 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)) + + // 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) + 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. + 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 config-fragment entry. + 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 config-fragment line. + 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 configLines { + if line == identityFileLine(fallbackB) { + lineCount++ + } + } + require.Equal(t, 1, lineCount, "fragment must not accumulate duplicate IdentityFile lines") +} + +func TestWriteKeysLegacyCommentFallback(t *testing.T) { + // 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) + + 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")...) + 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) + 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{}} + fallback, err := cmd.writeKeysToDestination("", pem, certBytes) + require.NoError(t, err) + 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} + 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) { + _, _, 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) + 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) { + // 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{} + 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) { + _, _, 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, "")) + 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. + cmd, afs, dirPath, identity, pem, certBytes := opkDirFixture(t) + + basePath := filepath.Join(dirPath, cmd.makeSSHKeyFileName(cmd.pkt)) + require.NoError(t, afs.WriteFile(basePath, []byte("orphan"), 0o600)) + + written, err := cmd.writeKeysToOpkSSHDir(pem, certBytes, identity) + require.NoError(t, err) + require.Equal(t, basePath+"-"+identityTag(identity), written) + require.Equal(t, []byte("orphan"), mustReadFile(t, afs, basePath), "orphaned private key must be untouched") +} + +func TestFragmentWholeLineDedup(t *testing.T) { + // 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)) + taggedLine := identityFileLine(basePath + "-" + identityTag(identity)) + require.NoError(t, afs.WriteFile(filepath.Join(dirPath, "config"), []byte(taggedLine+"\n"), 0o600)) + + written, err := cmd.writeKeysToOpkSSHDir(pem, certBytes, identity) + require.NoError(t, err) + require.Equal(t, basePath, written, "free base slot must be used") + + 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. + cmd, afs, dirPath, identity, pem, certBytes := opkDirFixture(t) + + 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, 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")) + + 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") + 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) +} diff --git a/commands/logout.go b/commands/logout.go index 49ed4207..6df2f669 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") + // 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 for _, line := range lines { if strings.TrimSpace(line) != identityLine { 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_login.md b/docs/cli/opkssh_login.md index bbbe1f74..6af58eff 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,13 +29,14 @@ 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 -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 diff --git a/docs/config.md b/docs/config.md index 867deec2..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). @@ -25,6 +35,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. 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/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 998794c3..ee0abc27 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, @@ -166,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: @@ -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 @@ -208,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", "", 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") @@ -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 @@ -480,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", "", config.ConfigPathFlagHelp) providerCmd.AddCommand(providerListCmd) diff --git a/test/integration/opkssh_test.go b/test/integration/opkssh_test.go index 8dae162d..85978e1c 100644 --- a/test/integration/opkssh_test.go +++ b/test/integration/opkssh_test.go @@ -39,6 +39,11 @@ func TestMain(m *testing.M) { defer cancel() TestCtx = ctx + // 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() }()) }