From a0b746778514bb0cef94e4520cebbd348d25d50b Mon Sep 17 00:00:00 2001 From: "F.D.Castel" Date: Wed, 25 Mar 2026 19:21:02 -0300 Subject: [PATCH 1/3] feat: add logout command to remove opkssh-generated SSH keys Adds a new 'opkssh logout' command that finds and removes SSH keys and certificates generated by opkssh. This addresses the user request in #317 for the ability to log out and destroy SSH certs. The logout command: - Scans ~/.ssh/ for default key files (id_ecdsa, id_ecdsa_sk, id_ed25519, id_ed25519_sk) and removes those with an 'openpubkey' comment - Scans ~/.ssh/opkssh/ for opkssh-managed keys and removes them, cleaning up the config file entries - Supports -i flag to target a specific key pair - Safely skips keys not generated by opkssh Closes #317 --- commands/logout.go | 251 ++++++++++++++++++++++++++++++++++ commands/logout_test.go | 293 ++++++++++++++++++++++++++++++++++++++++ main.go | 25 ++++ 3 files changed, 569 insertions(+) create mode 100644 commands/logout.go create mode 100644 commands/logout_test.go diff --git a/commands/logout.go b/commands/logout.go new file mode 100644 index 00000000..af49c01b --- /dev/null +++ b/commands/logout.go @@ -0,0 +1,251 @@ +// Copyright 2025 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 commands + +import ( + "fmt" + "io" + "os" + "path/filepath" + "strings" + + "github.com/spf13/afero" + "golang.org/x/crypto/ssh" +) + +// LogoutCmd represents the logout command that removes opkssh-generated SSH keys and certificates. +type LogoutCmd struct { + Fs afero.Fs + KeyPathArg string // Optional: specific key path to remove + OutWriter io.Writer +} + +// NewLogoutCmd creates a new LogoutCmd instance. +func NewLogoutCmd(keyPathArg string) *LogoutCmd { + return &LogoutCmd{ + Fs: afero.NewOsFs(), + KeyPathArg: keyPathArg, + } +} + +func (l *LogoutCmd) out() io.Writer { + if l.OutWriter != nil { + return l.OutWriter + } + return os.Stdout +} + +// Run executes the logout command, removing opkssh-generated SSH keys. +func (l *LogoutCmd) Run() error { + if l.KeyPathArg != "" { + return l.removeSpecificKey(l.KeyPathArg) + } + + removedCount := 0 + + // Remove keys from default SSH directory (~/.ssh/) + n, err := l.removeDefaultSSHDirKeys() + if err != nil { + return err + } + removedCount += n + + // Remove keys from opkssh directory (~/.ssh/opkssh/) + n, err = l.removeOpkSSHDirKeys() + if err != nil { + return err + } + removedCount += n + + if removedCount == 0 { + fmt.Fprintln(l.out(), "No opkssh keys found to remove") + } else { + fmt.Fprintf(l.out(), "Successfully removed %d opkssh key pair(s)\n", removedCount) + } + return nil +} + +// removeSpecificKey removes a specific key pair given the private key path. +func (l *LogoutCmd) removeSpecificKey(seckeyPath string) error { + pubkeyPath := seckeyPath + "-cert.pub" + + afs := &afero.Afero{Fs: l.Fs} + + // Check if the cert file exists and was generated by openpubkey + pubKeyBytes, err := afs.ReadFile(pubkeyPath) + if err != nil { + return fmt.Errorf("could not read certificate file %s: %w", pubkeyPath, err) + } + + if !isOpenpubkeyComment(pubKeyBytes) { + return fmt.Errorf("key at %s was not generated by opkssh", seckeyPath) + } + + if err := l.removeKeyPair(seckeyPath, pubkeyPath); err != nil { + return err + } + + fmt.Fprintf(l.out(), "Successfully removed opkssh key pair: %s\n", seckeyPath) + return nil +} + +// removeDefaultSSHDirKeys finds and removes opkssh-generated keys from ~/.ssh/. +func (l *LogoutCmd) removeDefaultSSHDirKeys() (int, error) { + homePath, err := os.UserHomeDir() + if err != nil { + return 0, fmt.Errorf("failed to get home directory: %w", err) + } + sshPath := filepath.Join(homePath, ".ssh") + + keyFileNames := []string{"id_ecdsa", "id_ecdsa_sk", "id_ed25519", "id_ed25519_sk"} + removedCount := 0 + + afs := &afero.Afero{Fs: l.Fs} + + for _, keyFilename := range keyFileNames { + seckeyPath := filepath.Join(sshPath, keyFilename) + pubkeyPath := seckeyPath + "-cert.pub" + + // Check if the cert file exists + pubKeyBytes, err := afs.ReadFile(pubkeyPath) + if err != nil { + continue // File doesn't exist or can't be read, skip + } + + if !isOpenpubkeyComment(pubKeyBytes) { + continue // Not generated by openpubkey, skip + } + + if err := l.removeKeyPair(seckeyPath, pubkeyPath); err != nil { + return removedCount, err + } + + fmt.Fprintf(l.out(), "Removed %s and %s\n", seckeyPath, pubkeyPath) + removedCount++ + } + + return removedCount, nil +} + +// removeOpkSSHDirKeys finds and removes opkssh-generated keys from ~/.ssh/opkssh/. +func (l *LogoutCmd) removeOpkSSHDirKeys() (int, error) { + homePath, err := os.UserHomeDir() + if err != nil { + return 0, fmt.Errorf("failed to get home directory: %w", err) + } + + opkSSHDir := filepath.Join(homePath, ".ssh", "opkssh") + afs := &afero.Afero{Fs: l.Fs} + + exists, err := afs.DirExists(opkSSHDir) + if err != nil || !exists { + return 0, nil + } + + entries, err := afs.ReadDir(opkSSHDir) + if err != nil { + return 0, nil + } + + removedCount := 0 + configPath := filepath.Join(opkSSHDir, "config") + + for _, entry := range entries { + if entry.IsDir() { + continue + } + name := entry.Name() + + // Skip config file and cert files (we handle them with their private key) + if name == "config" || strings.HasSuffix(name, "-cert.pub") { + continue + } + + seckeyPath := filepath.Join(opkSSHDir, name) + pubkeyPath := seckeyPath + "-cert.pub" + + // Check if the cert file exists + pubKeyBytes, err := afs.ReadFile(pubkeyPath) + if err != nil { + continue + } + + if !isOpenpubkeyComment(pubKeyBytes) { + continue + } + + if err := l.removeKeyPair(seckeyPath, pubkeyPath); err != nil { + return removedCount, err + } + + // Remove the IdentityFile entry from the opkssh config + if err := l.removeFromOpkSSHConfig(configPath, seckeyPath); err != nil { + return removedCount, fmt.Errorf("failed to update opkssh config: %w", err) + } + + fmt.Fprintf(l.out(), "Removed %s and %s\n", seckeyPath, pubkeyPath) + removedCount++ + } + + return removedCount, nil +} + +// removeKeyPair removes both the private key and certificate files. +func (l *LogoutCmd) removeKeyPair(seckeyPath string, pubkeyPath string) error { + // Remove certificate file first + if err := l.Fs.Remove(pubkeyPath); err != nil && !os.IsNotExist(err) { + return fmt.Errorf("failed to remove certificate %s: %w", pubkeyPath, err) + } + + // Remove private key file + if err := l.Fs.Remove(seckeyPath); err != nil && !os.IsNotExist(err) { + return fmt.Errorf("failed to remove key %s: %w", seckeyPath, err) + } + + return nil +} + +// removeFromOpkSSHConfig removes the IdentityFile line for the given key from the opkssh config file. +func (l *LogoutCmd) removeFromOpkSSHConfig(configPath string, seckeyPath string) error { + afs := &afero.Afero{Fs: l.Fs} + + content, err := afs.ReadFile(configPath) + if err != nil { + return nil // Config file doesn't exist, nothing to clean up + } + + identityLine := "IdentityFile " + seckeyPath + lines := strings.Split(string(content), "\n") + var newLines []string + for _, line := range lines { + if strings.TrimSpace(line) != identityLine { + newLines = append(newLines, line) + } + } + + newContent := strings.Join(newLines, "\n") + return afs.WriteFile(configPath, []byte(newContent), 0o600) +} + +// isOpenpubkeyComment checks if an SSH public key file has an openpubkey-generated comment. +func isOpenpubkeyComment(pubKeyBytes []byte) bool { + _, comment, _, _, err := ssh.ParseAuthorizedKey(pubKeyBytes) + if err != nil { + return false + } + return strings.HasPrefix(comment, "openpubkey") +} diff --git a/commands/logout_test.go b/commands/logout_test.go new file mode 100644 index 00000000..6f28debc --- /dev/null +++ b/commands/logout_test.go @@ -0,0 +1,293 @@ +// Copyright 2025 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 commands + +import ( + "bytes" + "os" + "path/filepath" + "testing" + + "github.com/spf13/afero" + "github.com/stretchr/testify/require" +) + +func setupLogoutTestKeys(t *testing.T, mockFs afero.Fs, keyType KeyType) (string, string) { + t.Helper() + + pkt, signer, _ := Mocks(t, keyType) + principals := []string{} + certBytes, seckeySshPem, err := createSSHCert(pkt, signer, principals) + require.NoError(t, err) + + homePath, err := os.UserHomeDir() + require.NoError(t, err) + sshPath := filepath.Join(homePath, ".ssh") + + afs := &afero.Afero{Fs: mockFs} + err = afs.MkdirAll(sshPath, os.ModePerm) + require.NoError(t, err) + + var keyFileName string + switch keyType { + case ECDSA: + keyFileName = "id_ecdsa" + case ED25519: + keyFileName = "id_ed25519" + } + + seckeyPath := filepath.Join(sshPath, keyFileName) + pubkeyPath := seckeyPath + "-cert.pub" + + err = afs.WriteFile(seckeyPath, seckeySshPem, 0o600) + require.NoError(t, err) + + // Append "openpubkey" comment like writeKeys does + certBytes = append(certBytes, []byte(" openpubkey")...) + err = afs.WriteFile(pubkeyPath, certBytes, 0o644) + require.NoError(t, err) + + return seckeyPath, pubkeyPath +} + +func TestLogoutCmd_RemoveDefaultKeys(t *testing.T) { + keyTypes := []KeyType{ECDSA, ED25519} + + for _, keyType := range keyTypes { + t.Run("removes "+keyType.String()+" keys", func(t *testing.T) { + mockFs := afero.NewMemMapFs() + seckeyPath, pubkeyPath := setupLogoutTestKeys(t, mockFs, keyType) + + output := &bytes.Buffer{} + logoutCmd := &LogoutCmd{ + Fs: mockFs, + OutWriter: output, + } + + err := logoutCmd.Run() + require.NoError(t, err) + + // Verify files were removed + _, err = mockFs.Stat(seckeyPath) + require.True(t, os.IsNotExist(err), "private key should be removed") + + _, err = mockFs.Stat(pubkeyPath) + require.True(t, os.IsNotExist(err), "certificate should be removed") + + require.Contains(t, output.String(), "Removed") + require.Contains(t, output.String(), "Successfully removed 1 opkssh key pair(s)") + }) + } +} + +func TestLogoutCmd_NoKeysFound(t *testing.T) { + mockFs := afero.NewMemMapFs() + output := &bytes.Buffer{} + logoutCmd := &LogoutCmd{ + Fs: mockFs, + OutWriter: output, + } + + err := logoutCmd.Run() + require.NoError(t, err) + require.Contains(t, output.String(), "No opkssh keys found to remove") +} + +func TestLogoutCmd_SkipsNonOpenpubkeyKeys(t *testing.T) { + mockFs := afero.NewMemMapFs() + afs := &afero.Afero{Fs: mockFs} + + homePath, err := os.UserHomeDir() + require.NoError(t, err) + sshPath := filepath.Join(homePath, ".ssh") + + err = afs.MkdirAll(sshPath, os.ModePerm) + require.NoError(t, err) + + // Create a non-openpubkey key pair by using Mocks but writing a different comment + pkt, signer, _ := Mocks(t, ECDSA) + principals := []string{} + certBytes, seckeySshPem, err := createSSHCert(pkt, signer, principals) + require.NoError(t, err) + + seckeyPath := filepath.Join(sshPath, "id_ecdsa") + pubkeyPath := seckeyPath + "-cert.pub" + + err = afs.WriteFile(seckeyPath, seckeySshPem, 0o600) + require.NoError(t, err) + + // Write cert with a non-openpubkey comment + certBytes = append(certBytes, []byte(" user@host")...) + err = afs.WriteFile(pubkeyPath, certBytes, 0o644) + require.NoError(t, err) + + output := &bytes.Buffer{} + logoutCmd := &LogoutCmd{ + Fs: mockFs, + OutWriter: output, + } + + err = logoutCmd.Run() + require.NoError(t, err) + require.Contains(t, output.String(), "No opkssh keys found to remove") + + // Verify files still exist + _, err = mockFs.Stat(seckeyPath) + require.NoError(t, err, "private key should still exist") + + _, err = mockFs.Stat(pubkeyPath) + require.NoError(t, err, "certificate should still exist") +} + +func TestLogoutCmd_SpecificKey(t *testing.T) { + mockFs := afero.NewMemMapFs() + seckeyPath, pubkeyPath := setupLogoutTestKeys(t, mockFs, ECDSA) + + output := &bytes.Buffer{} + logoutCmd := &LogoutCmd{ + Fs: mockFs, + KeyPathArg: seckeyPath, + OutWriter: output, + } + + err := logoutCmd.Run() + require.NoError(t, err) + + // Verify files were removed + _, err = mockFs.Stat(seckeyPath) + require.True(t, os.IsNotExist(err), "private key should be removed") + + _, err = mockFs.Stat(pubkeyPath) + require.True(t, os.IsNotExist(err), "certificate should be removed") + + require.Contains(t, output.String(), "Successfully removed opkssh key pair") +} + +func TestLogoutCmd_SpecificKeyNotOpenpubkey(t *testing.T) { + mockFs := afero.NewMemMapFs() + afs := &afero.Afero{Fs: mockFs} + + homePath, err := os.UserHomeDir() + require.NoError(t, err) + sshPath := filepath.Join(homePath, ".ssh") + + err = afs.MkdirAll(sshPath, os.ModePerm) + require.NoError(t, err) + + pkt, signer, _ := Mocks(t, ECDSA) + principals := []string{} + certBytes, seckeySshPem, err := createSSHCert(pkt, signer, principals) + require.NoError(t, err) + + seckeyPath := filepath.Join(sshPath, "id_ecdsa") + pubkeyPath := seckeyPath + "-cert.pub" + + err = afs.WriteFile(seckeyPath, seckeySshPem, 0o600) + require.NoError(t, err) + + certBytes = append(certBytes, []byte(" user@host")...) + err = afs.WriteFile(pubkeyPath, certBytes, 0o644) + require.NoError(t, err) + + output := &bytes.Buffer{} + logoutCmd := &LogoutCmd{ + Fs: mockFs, + KeyPathArg: seckeyPath, + OutWriter: output, + } + + err = logoutCmd.Run() + require.Error(t, err) + require.Contains(t, err.Error(), "was not generated by opkssh") +} + +func TestLogoutCmd_OpkSSHDir(t *testing.T) { + mockFs := afero.NewMemMapFs() + afs := &afero.Afero{Fs: mockFs} + + homePath, err := os.UserHomeDir() + require.NoError(t, err) + opkSSHDir := filepath.Join(homePath, ".ssh", "opkssh") + + err = afs.MkdirAll(opkSSHDir, 0o700) + require.NoError(t, err) + + pkt, signer, _ := Mocks(t, ECDSA) + principals := []string{} + certBytes, seckeySshPem, err := createSSHCert(pkt, signer, principals) + require.NoError(t, err) + + keyName := "accounts.example.com-test_client_id" + seckeyPath := filepath.Join(opkSSHDir, keyName) + pubkeyPath := seckeyPath + "-cert.pub" + + err = afs.WriteFile(seckeyPath, seckeySshPem, 0o600) + require.NoError(t, err) + + // Write cert with openpubkey comment (like writeKeysComment does) + certBytes = append(certBytes, []byte(" openpubkey: https://accounts.example.com test_client_id")...) + err = afs.WriteFile(pubkeyPath, certBytes, 0o644) + require.NoError(t, err) + + // Create config file with IdentityFile entry + configContent := "IdentityFile " + seckeyPath + "\n" + configPath := filepath.Join(opkSSHDir, "config") + err = afs.WriteFile(configPath, []byte(configContent), 0o600) + require.NoError(t, err) + + output := &bytes.Buffer{} + logoutCmd := &LogoutCmd{ + Fs: mockFs, + OutWriter: output, + } + + err = logoutCmd.Run() + require.NoError(t, err) + + // Verify key files were removed + _, err = mockFs.Stat(seckeyPath) + require.True(t, os.IsNotExist(err), "private key should be removed") + + _, err = mockFs.Stat(pubkeyPath) + require.True(t, os.IsNotExist(err), "certificate should be removed") + + // Verify config was cleaned up + configBytes, err := afs.ReadFile(configPath) + require.NoError(t, err) + require.NotContains(t, string(configBytes), seckeyPath) + + require.Contains(t, output.String(), "Removed") + require.Contains(t, output.String(), "Successfully removed 1 opkssh key pair(s)") +} + +func TestLogoutCmd_MultipleKeys(t *testing.T) { + mockFs := afero.NewMemMapFs() + + // Setup both ECDSA and ED25519 keys + setupLogoutTestKeys(t, mockFs, ECDSA) + setupLogoutTestKeys(t, mockFs, ED25519) + + output := &bytes.Buffer{} + logoutCmd := &LogoutCmd{ + Fs: mockFs, + OutWriter: output, + } + + err := logoutCmd.Run() + require.NoError(t, err) + require.Contains(t, output.String(), "Successfully removed 2 opkssh key pair(s)") +} diff --git a/main.go b/main.go index c980223e..bbef9bb4 100644 --- a/main.go +++ b/main.go @@ -216,6 +216,31 @@ Arguments: 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") rootCmd.AddCommand(loginCmd) + var logoutKeyPathArg string + logoutCmd := &cobra.Command{ + SilenceUsage: true, + Use: "logout", + Short: "Remove opkssh-generated SSH keys and certificates", + Long: `Logout removes SSH keys and certificates that were generated by opkssh. + +By default it searches the standard SSH key locations (~/.ssh/) and the opkssh identity directory (~/.ssh/opkssh/) for keys generated by opkssh and removes them. + +Use the -i flag to remove a specific key pair.`, + Example: ` opkssh logout + opkssh logout -i ~/.ssh/id_ecdsa`, + Args: cobra.NoArgs, + RunE: func(cmd *cobra.Command, args []string) error { + logout := commands.NewLogoutCmd(logoutKeyPathArg) + if err := logout.Run(); err != nil { + log.Println("Error executing logout command:", err) + return err + } + return nil + }, + } + logoutCmd.Flags().StringVarP(&logoutKeyPathArg, "private-key-file", "i", "", "Path to the specific private key to remove") + rootCmd.AddCommand(logoutCmd) + readhomeCmd := &cobra.Command{ SilenceUsage: true, Use: "readhome ", From b7b00ecbe67819ff74448b19c93d03bd5bd92929 Mon Sep 17 00:00:00 2001 From: "F.D.Castel" Date: Wed, 25 Mar 2026 19:28:23 -0300 Subject: [PATCH 2/3] fix: update copyright year in logout command files --- commands/logout.go | 2 +- commands/logout_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/commands/logout.go b/commands/logout.go index af49c01b..d4268734 100644 --- a/commands/logout.go +++ b/commands/logout.go @@ -1,4 +1,4 @@ -// Copyright 2025 OpenPubkey +// 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. diff --git a/commands/logout_test.go b/commands/logout_test.go index 6f28debc..43a2b5e3 100644 --- a/commands/logout_test.go +++ b/commands/logout_test.go @@ -1,4 +1,4 @@ -// Copyright 2025 OpenPubkey +// 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. From 1d21a273136072a15d6e509f64a3aaa5e87bf227 Mon Sep 17 00:00:00 2001 From: "F.D.Castel" Date: Thu, 26 Mar 2026 15:21:15 -0300 Subject: [PATCH 3/3] refactor: address PR review feedback for logout command Changes based on review by @EthanHeilman: - Validate cert matches secret key before deleting: verifyKeyPairMatch() compares the public key in the certificate against the secret key to prevent accidentally deleting a secret key when the public key has been overwritten with a different cert. On mismatch, prints warning to stderr and skips the key pair. - Remove secret key first, then certificate: reversed deletion order so if an error occurs mid-way, the secret key won't be left orphaned on disk (the cert can still be used to find it). - Share key file names with login: extracted DefaultSSHKeyFileNames map in login.go, used by both login and logout so adding a new key type to login automatically includes it in logout. - Handle Windows line endings: removeFromOpkSSHConfig now normalizes \r\n to \n before splitting lines. - Add verbose flag: -v/--verbose prints skip reasons to stderr for debugging (e.g. 'not generated by opkssh', 'could not read cert'). - Use 0o600 permission for config file writes (already correct, now documented in context). --- commands/login.go | 17 ++-- commands/logout.go | 121 ++++++++++++++++++++++--- commands/logout_test.go | 194 +++++++++++++++++++++++++++++++++++++++- main.go | 5 ++ 4 files changed, 318 insertions(+), 19 deletions(-) diff --git a/commands/login.go b/commands/login.go index 7152a01f..bdd15f1e 100644 --- a/commands/login.go +++ b/commands/login.go @@ -69,6 +69,14 @@ func (k KeyType) String() string { } } +// DefaultSSHKeyFileNames are the file names ssh key pairs that opkssh may +// write to in ~/.ssh/ during login. These are used by both login and logout +// so that if a new key type is added, logout will automatically pick it up. +var DefaultSSHKeyFileNames = map[KeyType][]string{ + ECDSA: {"id_ecdsa", "id_ecdsa_sk"}, + ED25519: {"id_ed25519", "id_ed25519_sk"}, +} + // LoginCmd represents the login command that performs OIDC authentication and generates SSH certificates. type LoginCmd struct { // Inputs @@ -734,13 +742,8 @@ func (l *LoginCmd) writeKeysToSSHDir(seckeySshPem []byte, certBytes []byte) erro // 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. - var keyFileNames []string - switch l.KeyTypeArg { - case ECDSA: - keyFileNames = []string{"id_ecdsa", "id_ecdsa_sk"} - case ED25519: - keyFileNames = []string{"id_ed25519", "id_ed25519_sk"} - default: + keyFileNames, ok := DefaultSSHKeyFileNames[l.KeyTypeArg] + if !ok { return fmt.Errorf("key type (%s) has no default output file name; use -i ", l.KeyTypeArg.String()) } diff --git a/commands/logout.go b/commands/logout.go index d4268734..49ed4207 100644 --- a/commands/logout.go +++ b/commands/logout.go @@ -1,4 +1,4 @@ -// Copyright 2026 OpenPubkey +// Copyright 2025 OpenPubkey // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -31,7 +31,9 @@ import ( type LogoutCmd struct { Fs afero.Fs KeyPathArg string // Optional: specific key path to remove + Verbosity int // Default verbosity is 0, 1 is verbose OutWriter io.Writer + ErrWriter io.Writer } // NewLogoutCmd creates a new LogoutCmd instance. @@ -49,6 +51,13 @@ func (l *LogoutCmd) out() io.Writer { return os.Stdout } +func (l *LogoutCmd) errOut() io.Writer { + if l.ErrWriter != nil { + return l.ErrWriter + } + return os.Stderr +} + // Run executes the logout command, removing opkssh-generated SSH keys. func (l *LogoutCmd) Run() error { if l.KeyPathArg != "" { @@ -95,6 +104,15 @@ func (l *LogoutCmd) removeSpecificKey(seckeyPath string) error { return fmt.Errorf("key at %s was not generated by opkssh", seckeyPath) } + // Verify that the certificate matches the secret key before deleting + secKeyBytes, err := afs.ReadFile(seckeyPath) + if err != nil { + return fmt.Errorf("could not read secret key file %s: %w", seckeyPath, err) + } + if err := verifyKeyPairMatch(secKeyBytes, pubKeyBytes); err != nil { + return fmt.Errorf("key pair mismatch for %s: %w", seckeyPath, err) + } + if err := l.removeKeyPair(seckeyPath, pubkeyPath); err != nil { return err } @@ -103,6 +121,15 @@ func (l *LogoutCmd) removeSpecificKey(seckeyPath string) error { return nil } +// allDefaultSSHKeyFileNames returns all key file names from DefaultSSHKeyFileNames. +func allDefaultSSHKeyFileNames() []string { + var all []string + for _, names := range DefaultSSHKeyFileNames { + all = append(all, names...) + } + return all +} + // removeDefaultSSHDirKeys finds and removes opkssh-generated keys from ~/.ssh/. func (l *LogoutCmd) removeDefaultSSHDirKeys() (int, error) { homePath, err := os.UserHomeDir() @@ -111,25 +138,40 @@ func (l *LogoutCmd) removeDefaultSSHDirKeys() (int, error) { } sshPath := filepath.Join(homePath, ".ssh") - keyFileNames := []string{"id_ecdsa", "id_ecdsa_sk", "id_ed25519", "id_ed25519_sk"} removedCount := 0 - afs := &afero.Afero{Fs: l.Fs} - for _, keyFilename := range keyFileNames { + for _, keyFilename := range allDefaultSSHKeyFileNames() { seckeyPath := filepath.Join(sshPath, keyFilename) pubkeyPath := seckeyPath + "-cert.pub" // Check if the cert file exists pubKeyBytes, err := afs.ReadFile(pubkeyPath) if err != nil { + if l.Verbosity >= 1 { + fmt.Fprintf(l.errOut(), "Skipping %s: could not read certificate file\n", pubkeyPath) + } continue // File doesn't exist or can't be read, skip } if !isOpenpubkeyComment(pubKeyBytes) { + if l.Verbosity >= 1 { + fmt.Fprintf(l.errOut(), "Skipping %s: not generated by opkssh\n", seckeyPath) + } continue // Not generated by openpubkey, skip } + // Verify that the certificate matches the secret key before deleting + secKeyBytes, err := afs.ReadFile(seckeyPath) + if err != nil { + fmt.Fprintf(l.errOut(), "Skipping %s: could not read secret key file\n", seckeyPath) + continue + } + if err := verifyKeyPairMatch(secKeyBytes, pubKeyBytes); err != nil { + fmt.Fprintf(l.errOut(), "Skipping %s: certificate does not match secret key\n", seckeyPath) + continue + } + if err := l.removeKeyPair(seckeyPath, pubkeyPath); err != nil { return removedCount, err } @@ -181,10 +223,27 @@ func (l *LogoutCmd) removeOpkSSHDirKeys() (int, error) { // Check if the cert file exists pubKeyBytes, err := afs.ReadFile(pubkeyPath) if err != nil { + if l.Verbosity >= 1 { + fmt.Fprintf(l.errOut(), "Skipping %s: could not read certificate file\n", pubkeyPath) + } continue } if !isOpenpubkeyComment(pubKeyBytes) { + if l.Verbosity >= 1 { + fmt.Fprintf(l.errOut(), "Skipping %s: not generated by opkssh\n", seckeyPath) + } + continue + } + + // Verify that the certificate matches the secret key before deleting + secKeyBytes, err := afs.ReadFile(seckeyPath) + if err != nil { + fmt.Fprintf(l.errOut(), "Skipping %s: could not read secret key file\n", seckeyPath) + continue + } + if err := verifyKeyPairMatch(secKeyBytes, pubKeyBytes); err != nil { + fmt.Fprintf(l.errOut(), "Skipping %s: certificate does not match secret key\n", seckeyPath) continue } @@ -205,17 +264,19 @@ func (l *LogoutCmd) removeOpkSSHDirKeys() (int, error) { } // removeKeyPair removes both the private key and certificate files. +// The secret key is removed first so that if an error occurs removing the +// certificate, the secret key will not be left orphaned on disk. func (l *LogoutCmd) removeKeyPair(seckeyPath string, pubkeyPath string) error { - // Remove certificate file first - if err := l.Fs.Remove(pubkeyPath); err != nil && !os.IsNotExist(err) { - return fmt.Errorf("failed to remove certificate %s: %w", pubkeyPath, err) - } - - // Remove private key file + // Remove secret key first to avoid leaving it orphaned if cert removal fails if err := l.Fs.Remove(seckeyPath); err != nil && !os.IsNotExist(err) { return fmt.Errorf("failed to remove key %s: %w", seckeyPath, err) } + // Remove certificate file + if err := l.Fs.Remove(pubkeyPath); err != nil && !os.IsNotExist(err) { + return fmt.Errorf("failed to remove certificate %s: %w", pubkeyPath, err) + } + return nil } @@ -229,7 +290,10 @@ func (l *LogoutCmd) removeFromOpkSSHConfig(configPath string, seckeyPath string) } identityLine := "IdentityFile " + seckeyPath - lines := strings.Split(string(content), "\n") + + // Split handling both \r\n (Windows) and \n (Unix) line endings + normalized := strings.ReplaceAll(string(content), "\r\n", "\n") + lines := strings.Split(normalized, "\n") var newLines []string for _, line := range lines { if strings.TrimSpace(line) != identityLine { @@ -241,6 +305,41 @@ func (l *LogoutCmd) removeFromOpkSSHConfig(configPath string, seckeyPath string) return afs.WriteFile(configPath, []byte(newContent), 0o600) } +// verifyKeyPairMatch checks that the public key in the certificate corresponds +// to the given secret key. This prevents accidentally deleting a secret key +// when someone has overwritten the public key file with a different cert. +func verifyKeyPairMatch(secKeyBytes []byte, pubKeyBytes []byte) error { + secKey, err := ssh.ParsePrivateKey(secKeyBytes) + if err != nil { + return fmt.Errorf("failed to parse secret key: %w", err) + } + + pubKey, _, _, _, err := ssh.ParseAuthorizedKey(pubKeyBytes) + if err != nil { + return fmt.Errorf("failed to parse certificate: %w", err) + } + + // If the pubKey is a certificate, extract the underlying key + cert, ok := pubKey.(*ssh.Certificate) + if !ok { + return fmt.Errorf("public key file does not contain a certificate") + } + + // Compare the public key from the certificate with the public key derived from the secret key + secPubKey := secKey.PublicKey() + certPubKey := cert.Key + + if secPubKey.Type() != certPubKey.Type() { + return fmt.Errorf("key type mismatch: secret key is %s, certificate key is %s", secPubKey.Type(), certPubKey.Type()) + } + + if string(secPubKey.Marshal()) != string(certPubKey.Marshal()) { + return fmt.Errorf("public key in certificate does not match secret key") + } + + return nil +} + // isOpenpubkeyComment checks if an SSH public key file has an openpubkey-generated comment. func isOpenpubkeyComment(pubKeyBytes []byte) bool { _, comment, _, _, err := ssh.ParseAuthorizedKey(pubKeyBytes) diff --git a/commands/logout_test.go b/commands/logout_test.go index 43a2b5e3..86a60863 100644 --- a/commands/logout_test.go +++ b/commands/logout_test.go @@ -1,4 +1,4 @@ -// Copyright 2026 OpenPubkey +// Copyright 2025 OpenPubkey // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -76,6 +76,7 @@ func TestLogoutCmd_RemoveDefaultKeys(t *testing.T) { logoutCmd := &LogoutCmd{ Fs: mockFs, OutWriter: output, + ErrWriter: &bytes.Buffer{}, } err := logoutCmd.Run() @@ -100,6 +101,7 @@ func TestLogoutCmd_NoKeysFound(t *testing.T) { logoutCmd := &LogoutCmd{ Fs: mockFs, OutWriter: output, + ErrWriter: &bytes.Buffer{}, } err := logoutCmd.Run() @@ -139,6 +141,7 @@ func TestLogoutCmd_SkipsNonOpenpubkeyKeys(t *testing.T) { logoutCmd := &LogoutCmd{ Fs: mockFs, OutWriter: output, + ErrWriter: &bytes.Buffer{}, } err = logoutCmd.Run() @@ -162,6 +165,7 @@ func TestLogoutCmd_SpecificKey(t *testing.T) { Fs: mockFs, KeyPathArg: seckeyPath, OutWriter: output, + ErrWriter: &bytes.Buffer{}, } err := logoutCmd.Run() @@ -208,6 +212,7 @@ func TestLogoutCmd_SpecificKeyNotOpenpubkey(t *testing.T) { Fs: mockFs, KeyPathArg: seckeyPath, OutWriter: output, + ErrWriter: &bytes.Buffer{}, } err = logoutCmd.Run() @@ -253,6 +258,7 @@ func TestLogoutCmd_OpkSSHDir(t *testing.T) { logoutCmd := &LogoutCmd{ Fs: mockFs, OutWriter: output, + ErrWriter: &bytes.Buffer{}, } err = logoutCmd.Run() @@ -285,9 +291,195 @@ func TestLogoutCmd_MultipleKeys(t *testing.T) { logoutCmd := &LogoutCmd{ Fs: mockFs, OutWriter: output, + ErrWriter: &bytes.Buffer{}, } err := logoutCmd.Run() require.NoError(t, err) require.Contains(t, output.String(), "Successfully removed 2 opkssh key pair(s)") } + +func TestLogoutCmd_MismatchedKeyPair(t *testing.T) { + mockFs := afero.NewMemMapFs() + afs := &afero.Afero{Fs: mockFs} + + homePath, err := os.UserHomeDir() + require.NoError(t, err) + sshPath := filepath.Join(homePath, ".ssh") + + err = afs.MkdirAll(sshPath, os.ModePerm) + require.NoError(t, err) + + // Create a cert with one key pair + pkt1, signer1, _ := Mocks(t, ECDSA) + principals := []string{} + certBytes, _, err := createSSHCert(pkt1, signer1, principals) + require.NoError(t, err) + + // Create a different secret key + _, signer2, _ := Mocks(t, ECDSA) + _, seckeySshPem2, err := createSSHCert(pkt1, signer2, principals) + require.NoError(t, err) + + seckeyPath := filepath.Join(sshPath, "id_ecdsa") + pubkeyPath := seckeyPath + "-cert.pub" + + // Write the secret key from signer2 but cert from signer1 + err = afs.WriteFile(seckeyPath, seckeySshPem2, 0o600) + require.NoError(t, err) + + certBytes = append(certBytes, []byte(" openpubkey")...) + err = afs.WriteFile(pubkeyPath, certBytes, 0o644) + require.NoError(t, err) + + output := &bytes.Buffer{} + errOutput := &bytes.Buffer{} + logoutCmd := &LogoutCmd{ + Fs: mockFs, + OutWriter: output, + ErrWriter: errOutput, + } + + err = logoutCmd.Run() + require.NoError(t, err) // Should not error, just skip the mismatched pair + + // Verify files still exist (not deleted due to mismatch) + _, err = mockFs.Stat(seckeyPath) + require.NoError(t, err, "private key should still exist") + + _, err = mockFs.Stat(pubkeyPath) + require.NoError(t, err, "certificate should still exist") + + require.Contains(t, errOutput.String(), "certificate does not match secret key") + require.Contains(t, output.String(), "No opkssh keys found to remove") +} + +func TestLogoutCmd_MismatchedKeyPairSpecific(t *testing.T) { + mockFs := afero.NewMemMapFs() + afs := &afero.Afero{Fs: mockFs} + + homePath, err := os.UserHomeDir() + require.NoError(t, err) + sshPath := filepath.Join(homePath, ".ssh") + + err = afs.MkdirAll(sshPath, os.ModePerm) + require.NoError(t, err) + + // Create a cert with one key pair + pkt1, signer1, _ := Mocks(t, ECDSA) + principals := []string{} + certBytes, _, err := createSSHCert(pkt1, signer1, principals) + require.NoError(t, err) + + // Create a different secret key + _, signer2, _ := Mocks(t, ECDSA) + _, seckeySshPem2, err := createSSHCert(pkt1, signer2, principals) + require.NoError(t, err) + + seckeyPath := filepath.Join(sshPath, "id_ecdsa") + pubkeyPath := seckeyPath + "-cert.pub" + + err = afs.WriteFile(seckeyPath, seckeySshPem2, 0o600) + require.NoError(t, err) + + certBytes = append(certBytes, []byte(" openpubkey")...) + err = afs.WriteFile(pubkeyPath, certBytes, 0o644) + require.NoError(t, err) + + output := &bytes.Buffer{} + logoutCmd := &LogoutCmd{ + Fs: mockFs, + KeyPathArg: seckeyPath, + OutWriter: output, + ErrWriter: &bytes.Buffer{}, + } + + err = logoutCmd.Run() + require.Error(t, err) + require.Contains(t, err.Error(), "key pair mismatch") + + // Verify files still exist + _, err = mockFs.Stat(seckeyPath) + require.NoError(t, err, "private key should still exist") + + _, err = mockFs.Stat(pubkeyPath) + require.NoError(t, err, "certificate should still exist") +} + +func TestLogoutCmd_VerboseOutput(t *testing.T) { + mockFs := afero.NewMemMapFs() + afs := &afero.Afero{Fs: mockFs} + + homePath, err := os.UserHomeDir() + require.NoError(t, err) + sshPath := filepath.Join(homePath, ".ssh") + + err = afs.MkdirAll(sshPath, os.ModePerm) + require.NoError(t, err) + + // Create a non-openpubkey key so we can see the verbose skip message + pkt, signer, _ := Mocks(t, ECDSA) + principals := []string{} + certBytes, seckeySshPem, err := createSSHCert(pkt, signer, principals) + require.NoError(t, err) + + seckeyPath := filepath.Join(sshPath, "id_ecdsa") + pubkeyPath := seckeyPath + "-cert.pub" + + err = afs.WriteFile(seckeyPath, seckeySshPem, 0o600) + require.NoError(t, err) + + certBytes = append(certBytes, []byte(" user@host")...) + err = afs.WriteFile(pubkeyPath, certBytes, 0o644) + require.NoError(t, err) + + output := &bytes.Buffer{} + errOutput := &bytes.Buffer{} + logoutCmd := &LogoutCmd{ + Fs: mockFs, + Verbosity: 1, + OutWriter: output, + ErrWriter: errOutput, + } + + err = logoutCmd.Run() + require.NoError(t, err) + require.Contains(t, errOutput.String(), "not generated by opkssh") +} + +func TestVerifyKeyPairMatch(t *testing.T) { + t.Run("matching ECDSA pair", func(t *testing.T) { + pkt, signer, _ := Mocks(t, ECDSA) + certBytes, secKeyPem, err := createSSHCert(pkt, signer, []string{}) + require.NoError(t, err) + + certBytes = append(certBytes, []byte(" openpubkey")...) + err = verifyKeyPairMatch(secKeyPem, certBytes) + require.NoError(t, err) + }) + + t.Run("matching ED25519 pair", func(t *testing.T) { + pkt, signer, _ := Mocks(t, ED25519) + certBytes, secKeyPem, err := createSSHCert(pkt, signer, []string{}) + require.NoError(t, err) + + certBytes = append(certBytes, []byte(" openpubkey")...) + err = verifyKeyPairMatch(secKeyPem, certBytes) + require.NoError(t, err) + }) + + t.Run("mismatched pair", func(t *testing.T) { + pkt, signer1, _ := Mocks(t, ECDSA) + certBytes, _, err := createSSHCert(pkt, signer1, []string{}) + require.NoError(t, err) + + _, signer2, _ := Mocks(t, ECDSA) + _, secKeyPem2, err := createSSHCert(pkt, signer2, []string{}) + require.NoError(t, err) + + certBytes = append(certBytes, []byte(" openpubkey")...) + err = verifyKeyPairMatch(secKeyPem2, certBytes) + require.Error(t, err) + require.Contains(t, err.Error(), "does not match") + }) +} diff --git a/main.go b/main.go index bbef9bb4..a07b15f0 100644 --- a/main.go +++ b/main.go @@ -217,6 +217,7 @@ Arguments: rootCmd.AddCommand(loginCmd) var logoutKeyPathArg string + var logoutVerboseArg bool logoutCmd := &cobra.Command{ SilenceUsage: true, Use: "logout", @@ -231,6 +232,9 @@ Use the -i flag to remove a specific key pair.`, Args: cobra.NoArgs, RunE: func(cmd *cobra.Command, args []string) error { logout := commands.NewLogoutCmd(logoutKeyPathArg) + if logoutVerboseArg { + logout.Verbosity = 1 + } if err := logout.Run(); err != nil { log.Println("Error executing logout command:", err) return err @@ -239,6 +243,7 @@ Use the -i flag to remove a specific key pair.`, }, } logoutCmd.Flags().StringVarP(&logoutKeyPathArg, "private-key-file", "i", "", "Path to the specific private key to remove") + logoutCmd.Flags().BoolVarP(&logoutVerboseArg, "verbose", "v", false, "Print verbose output to stderr") rootCmd.AddCommand(logoutCmd) readhomeCmd := &cobra.Command{