diff --git a/cmd/internal/agentworkspace/setup_gpg.go b/cmd/internal/agentworkspace/setup_gpg.go index f8c3808ea..a8bb279c7 100644 --- a/cmd/internal/agentworkspace/setup_gpg.go +++ b/cmd/internal/agentworkspace/setup_gpg.go @@ -113,7 +113,7 @@ func fetchAndDecodeKeys(ownerTrustB64 string) ([]byte, []byte, error) { func configureGPGAgent(gpgConf *gpg.GPGConf) error { log.Debugf("Stopping container gpg-agent") - if err := gpgConf.StopGpgAgent(); err != nil { + if err := gpg.StopGpgAgent(); err != nil { return fmt.Errorf("stop container gpg-agent: %w", err) } @@ -135,7 +135,7 @@ func configureGPGAgent(gpgConf *gpg.GPGConf) error { // Now we again kill the agent and remove the socket to really be sure every // thing is clean log.Debugf("Ensure stopping container gpg-agent") - if err := gpgConf.StopGpgAgent(); err != nil { + if err := gpg.StopGpgAgent(); err != nil { return fmt.Errorf("ensure stopping container gpg-agent: %w", err) } @@ -145,7 +145,7 @@ func configureGPGAgent(gpgConf *gpg.GPGConf) error { } log.Debugf("Setup gpg.conf") - if err := gpgConf.SetupGpgConf(); err != nil { + if err := gpg.SetupGpgConf(); err != nil { return fmt.Errorf("setup gpg.conf: %w", err) } diff --git a/cmd/workspace/ssh.go b/cmd/workspace/ssh.go index 08f30ac13..447e62874 100644 --- a/cmd/workspace/ssh.go +++ b/cmd/workspace/ssh.go @@ -745,7 +745,10 @@ func (cmd *SSHCmd) setupGPGAgent( gitKey := gpg.SigningKey(ctx) - cmd.ReverseForwardPorts = append(cmd.ReverseForwardPorts, gpgExtraSocketPath) + cmd.ReverseForwardPorts = append( + cmd.ReverseForwardPorts, + gpg.ContainerSocketPath+":"+gpgExtraSocketPath, + ) // Now we forward the agent socket to the remote, and setup remote gpg to use it forwardAgent := []string{ @@ -757,7 +760,7 @@ func (cmd *SSHCmd) setupGPGAgent( names.Flag(names.OwnerTrust), ownerTrustArgument, names.Flag(names.SocketPath), - gpgExtraSocketPath, + gpg.ContainerSocketPath, } if log.DebugEnabled() { diff --git a/e2e/framework/command.go b/e2e/framework/command.go index 8952fdab8..499c35aee 100644 --- a/e2e/framework/command.go +++ b/e2e/framework/command.go @@ -468,34 +468,41 @@ func (f *Framework) SetupGPG(tmpDir string) error { return exec.Command("gpg", "-k").Run() } -func (f *Framework) DevsySSHGpgTestKey(ctx context.Context, workspace string) error { - pubKeyB, err := exec.Command("sh", "-c", "gpg -k --with-colons 2>/dev/null | grep sec | base64 -w0"). - Output() +func ImportGpgKey(privateKeyPath string) error { + // #nosec G204 -- test imports a controlled key path + out, err := exec.Command("gpg", "--batch", "--import", privateKeyPath).CombinedOutput() if err != nil { - return err + return fmt.Errorf("import gpg private key: %s: %w", out, err) + } + if out, err := exec.Command("gpg-connect-agent", "/bye").CombinedOutput(); err != nil { + return fmt.Errorf("start gpg-agent: %s: %w", out, err) } + return nil +} - // First run to trigger the first forwarding +func (f *Framework) DevsySSHGpgSecretKeyForwarded( + ctx context.Context, + workspace, expectedFingerprint string, +) error { stdout, _, err := f.ExecCommandCapture(ctx, []string{ cmdWorkspace, cmdSSH, names.Flag(names.AgentForwarding), names.Flag(names.SSHGPGForwarding), flagCommand, - "gpg -k --with-colons 2>/dev/null |grep sec | base64 -w0", workspace, + "gpg --list-secret-keys --with-colons 2>/dev/null", + workspace, }) if err != nil { - return err + return fmt.Errorf("list secret keys in container: %w", err) } - - if stdout != string(pubKeyB) { + if !strings.Contains(stdout, expectedFingerprint) { return fmt.Errorf( - "devsy gpg public key forwarding failed, expected %s, got %s", - string(pubKeyB), + "gpg agent forwarding failed: secret key %s not reachable in container, got: %q", + expectedFingerprint, stdout, ) } - return nil } diff --git a/e2e/tests/ssh/ssh.go b/e2e/tests/ssh/ssh.go index a5823330b..ae081679e 100644 --- a/e2e/tests/ssh/ssh.go +++ b/e2e/tests/ssh/ssh.go @@ -22,6 +22,8 @@ import ( const ( osWindows = "windows" + gpgTestKeyFingerprint = "07F681B9FD6C3411F679BFD1F51769DB572DDD3F" + envSSHAuthSock = "SSH_AUTH_SOCK" sshOptStrictHostKeyCheckingNo = "StrictHostKeyChecking=no" @@ -120,6 +122,43 @@ var _ = ginkgo.Describe("devsy ssh test suite", ginkgo.Label("ssh"), ginkgo.Orde }, ) + ginkgo.It( + "should expose the host GPG secret key in the container via agent forwarding", + ginkgo.Label("gpg"), + ginkgo.SpecTimeout(framework.TimeoutModerate()), + func(ctx ginkgo.SpecContext) { + if runtime.GOOS == osWindows { + ginkgo.Skip("skipping on windows") + } + + tempDir, err := framework.CopyToTempDir("tests/ssh/testdata/gpg-forwarding") + framework.ExpectNoError(err) + + f := framework.NewDefaultFramework(initialDir + "/bin") + _ = f.DevsyProviderAdd(ctx, "docker") + err = f.DevsyProviderUse(ctx, "docker") + framework.ExpectNoError(err) + + ginkgo.DeferCleanup(func(cleanupCtx context.Context) { + _ = f.DevsyWorkspaceDelete(cleanupCtx, tempDir) + framework.CleanupTempDir(initialDir, tempDir) + }) + + ginkgo.GinkgoT().Setenv("GNUPGHOME", ginkgo.GinkgoT().TempDir()) + framework.ExpectNoError( + framework.ImportGpgKey(filepath.Join(tempDir, "gpg-private.key")), + ) + + err = f.DevsyUp(ctx, tempDir, names.Flag(names.SSHGPGForwarding)) + framework.ExpectNoError(err) + + sshCtx, cancelSSH := context.WithDeadline(ctx, time.Now().Add(30*time.Second)) + defer cancelSSH() + err = f.DevsySSHGpgSecretKeyForwarded(sshCtx, tempDir, gpgTestKeyFingerprint) + framework.ExpectNoError(err) + }, + ) + ginkgo.It( "should set up git SSH signature helper and sign a commit", ginkgo.SpecTimeout(framework.TimeoutLong()), diff --git a/e2e/tests/ssh/testdata/gpg-forwarding/.devcontainer.json b/e2e/tests/ssh/testdata/gpg-forwarding/.devcontainer.json index b86c3547f..3b59e8273 100644 --- a/e2e/tests/ssh/testdata/gpg-forwarding/.devcontainer.json +++ b/e2e/tests/ssh/testdata/gpg-forwarding/.devcontainer.json @@ -1 +1 @@ -{ "image": "ghcr.io/devsy-org/test-images/base:ubuntu" } +{ "image": "ghcr.io/devsy-org/test-images/base:ubuntu", "remoteUser": "vscode" } diff --git a/e2e/tests/ssh/testdata/gpg-forwarding/devcontainer.json b/e2e/tests/ssh/testdata/gpg-forwarding/devcontainer.json index 25e8ab286..77ba88e1c 100755 --- a/e2e/tests/ssh/testdata/gpg-forwarding/devcontainer.json +++ b/e2e/tests/ssh/testdata/gpg-forwarding/devcontainer.json @@ -1,4 +1,5 @@ { "name": "Go", - "image": "ghcr.io/devsy-org/test-images/go:1" + "image": "ghcr.io/devsy-org/test-images/go:1", + "remoteUser": "vscode" } diff --git a/pkg/gpg/forward.go b/pkg/gpg/forward.go index 2d6d30e6c..24d676968 100644 --- a/pkg/gpg/forward.go +++ b/pkg/gpg/forward.go @@ -1,8 +1,10 @@ package gpg import ( + "context" "os" "os/exec" + "time" client2 "github.com/devsy-org/devsy/pkg/client" "github.com/devsy-org/devsy/pkg/flags/names" @@ -10,10 +12,15 @@ import ( devssh "github.com/devsy-org/devsy/pkg/ssh" ) -// ForwardAgent starts a background SSH connection that forwards the local GPG agent. -func ForwardAgent(client client2.BaseWorkspaceClient) error { - log.Debug("gpg forwarding enabled, performing immediately") +type backoff struct { + min, max time.Duration +} + +var forwardRestartBackoff = backoff{min: time.Second, max: 30 * time.Second} +// ForwardAgent starts a supervised background SSH connection that forwards the +// local GPG agent, restarting it until ctx is cancelled. +func ForwardAgent(ctx context.Context, client client2.BaseWorkspaceClient) error { execPath, err := os.Executable() if err != nil { return err @@ -32,14 +39,37 @@ func ForwardAgent(client client2.BaseWorkspaceClient) error { args := buildForwardArgs(remoteUser, client.Context(), client.Workspace()) - go func() { + go superviseForward(ctx, execPath, args, forwardRestartBackoff) + + return nil +} + +func superviseForward(ctx context.Context, execPath string, args []string, b backoff) { + delay := b.min + for { + if ctx.Err() != nil { + return + } + + start := time.Now() //nolint:gosec // execPath comes from os.Executable() - if runErr := exec.Command(execPath, args...).Run(); runErr != nil { - log.Errorf("failure in forwarding gpg-agent: %v", runErr) + runErr := exec.CommandContext(ctx, execPath, args...).Run() + if ctx.Err() != nil { + return } - }() - return nil + if time.Since(start) >= b.max { + delay = b.min + } + log.Errorf("gpg-agent forward exited (%v); restarting in %s", runErr, delay) + + select { + case <-ctx.Done(): + return + case <-time.After(delay): + } + delay = min(2*delay, b.max) + } } func buildForwardArgs(user, context, workspace string) []string { diff --git a/pkg/gpg/forward_test.go b/pkg/gpg/forward_test.go index 0c534e8ee..7786c9ba9 100644 --- a/pkg/gpg/forward_test.go +++ b/pkg/gpg/forward_test.go @@ -1,9 +1,15 @@ package gpg import ( + "context" + "os" + "path/filepath" + "strings" "testing" + "time" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestBuildForwardArgs(t *testing.T) { @@ -22,3 +28,46 @@ func TestBuildForwardArgs(t *testing.T) { } assert.Equal(t, expected, got) } + +func TestSuperviseForward_RestartsUntilCancelled(t *testing.T) { + runs := filepath.Join(t.TempDir(), "runs") + ctx, cancel := context.WithCancel(context.Background()) + + done := make(chan struct{}) + go func() { + superviseForward(ctx, "/bin/sh", []string{"-c", "printf x >> " + runs}, + backoff{min: 10 * time.Millisecond, max: 20 * time.Millisecond}) + close(done) + }() + + require.Eventually(t, func() bool { + data, _ := os.ReadFile(runs) //nolint:gosec // test path is created by the test + return strings.Count(string(data), "x") >= 2 + }, 5*time.Second, 10*time.Millisecond, "forward should be restarted after it exits") + + cancel() + + select { + case <-done: + case <-time.After(2 * time.Second): + t.Fatal("superviseForward did not stop after ctx cancel") + } +} + +func TestSuperviseForward_StopsImmediatelyIfCancelled(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + done := make(chan struct{}) + go func() { + superviseForward(ctx, "/bin/sh", []string{"-c", "exit 0"}, + backoff{min: 10 * time.Millisecond, max: 20 * time.Millisecond}) + close(done) + }() + + select { + case <-done: + case <-time.After(time.Second): + t.Fatal("superviseForward should return promptly when ctx is already cancelled") + } +} diff --git a/pkg/gpg/gpg_forwarding.go b/pkg/gpg/gpg_forwarding.go index baffb9afa..58174b2c9 100644 --- a/pkg/gpg/gpg_forwarding.go +++ b/pkg/gpg/gpg_forwarding.go @@ -9,7 +9,9 @@ import ( "path/filepath" "strconv" "strings" + "time" + "al.essio.dev/pkg/shellescape" "github.com/devsy-org/devsy/pkg/log" devssh "github.com/devsy-org/devsy/pkg/ssh" "golang.org/x/crypto/ssh" @@ -32,10 +34,10 @@ func IsGpgTunnelRunning( command := "gpg -K" if user != "" && user != "root" { - command = fmt.Sprintf("su -c \"%s\" '%s'", command, user) + command = shellescape.QuoteCommand([]string{"su", "-c", command, user}) } - // capture the output, if it's empty it means we don't have gpg-forwarding + // empty output means the forwarded agent exposes no secret keys var out bytes.Buffer err := devssh.Run(ctx, devssh.RunOptions{ Client: client, @@ -44,7 +46,7 @@ func IsGpgTunnelRunning( Stderr: writer, }) - return err == nil && len(out.Bytes()) > 1 + return err == nil && strings.TrimSpace(out.String()) != "" } func GetHostPubKey() ([]byte, error) { @@ -55,70 +57,68 @@ func GetHostOwnerTrust() ([]byte, error) { return exec.Command("gpg", "--export-ownertrust").Output() } -func (g *GPGConf) StopGpgAgent() error { - return exec.Command("gpgconf", []string{"--kill", "gpg-agent"}...).Run() +func StopGpgAgent() error { + return exec.Command("gpgconf", "--kill", "gpg-agent").Run() } func (g *GPGConf) ImportGpgKey() error { - gpgImportCmd := exec.Command("gpg", "--import") + return runGpgWithStdin(g.PublicKey, "--import") +} + +func (g *GPGConf) ImportOwnerTrust() error { + return runGpgWithStdin(g.OwnerTrust, "--import-ownertrust") +} - stdin, err := gpgImportCmd.StdinPipe() +func runGpgWithStdin(input []byte, args ...string) error { + //nolint:gosec // args are internal gpg directive literals + cmd := exec.Command("gpg", args...) + + stdin, err := cmd.StdinPipe() if err != nil { return err } - go func() { defer func() { _ = stdin.Close() }() - _, _ = stdin.Write(g.PublicKey) + _, _ = stdin.Write(input) }() - out, err := gpgImportCmd.CombinedOutput() + out, err := cmd.CombinedOutput() if err != nil { - return fmt.Errorf("import gpg public key: %s: %w", out, err) + return fmt.Errorf("gpg %s: %s: %w", strings.Join(args, " "), out, err) } - return nil } -func (g *GPGConf) ImportOwnerTrust() error { - gpgOwnerTrustCmd := exec.Command("gpg", "--import-ownertrust") +var gpgConfDirectives = []string{"use-agent", "no-autostart"} - stdin, err := gpgOwnerTrustCmd.StdinPipe() - if err != nil { +func SetupGpgConf() error { + existing, err := os.ReadFile(gpgConfigPath()) + if err != nil && !os.IsNotExist(err) { return err } - go func() { - defer func() { _ = stdin.Close() }() - _, _ = stdin.Write(g.OwnerTrust) - }() - - return gpgOwnerTrustCmd.Run() -} - -func (g *GPGConf) SetupGpgConf() error { - _, err := os.Stat(g.getConfigPath()) - if err != nil { - _, err = os.Create(g.getConfigPath()) - if err != nil { - return err - } - } - - gpgConfig, err := os.ReadFile(g.getConfigPath()) + f, err := os.OpenFile(gpgConfigPath(), os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0o600) if err != nil { return err } + defer func() { _ = f.Close() }() - if !strings.Contains(string(gpgConfig), "use-agent") { - f, err := os.OpenFile(g.getConfigPath(), - os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0o600) - if err != nil { - return err - } - defer func() { _ = f.Close() }() + return appendMissingDirectives(f, string(existing)) +} + +func appendMissingDirectives(f *os.File, existing string) error { + needsLeadingNewline := existing != "" && !strings.HasSuffix(existing, "\n") - if _, err := f.WriteString("use-agent\n"); err != nil { + for _, directive := range gpgConfDirectives { + if containsDirective(existing, directive) { + continue + } + line := directive + "\n" + if needsLeadingNewline { + line = "\n" + line + needsLeadingNewline = false + } + if _, err := f.WriteString(line); err != nil { return err } } @@ -126,56 +126,70 @@ func (g *GPGConf) SetupGpgConf() error { return nil } +func containsDirective(config, directive string) bool { + for line := range strings.SplitSeq(config, "\n") { + if strings.TrimSpace(line) == directive { + return true + } + } + return false +} + +// ContainerSocketPath is the container-local path the host gpg-agent socket is +// forwarded to; any workspace user can reach it. +const ContainerSocketPath = "/tmp/S.gpg-agent" + func (g *GPGConf) SetupRemoteSocketDirTree() error { - err := exec.Command("sudo", "mkdir", "-p", "/run/user", filepath.Dir(g.SocketPath)).Run() - if err != nil { + runUserDir := filepath.Join("/run/user", strconv.Itoa(os.Getuid())) + + //nolint:gosec // runUserDir is a fixed per-uid runtime path, not user input + if err := exec.Command("sudo", "mkdir", "-p", runUserDir).Run(); err != nil { return err } + //nolint:gosec // runUserDir is a fixed per-uid runtime path, not user input return exec.Command("sudo", - "chown", - "-R", + "chown", "-R", strconv.Itoa(os.Getuid())+":"+strconv.Itoa(os.Getgid()), - "/run/user", - filepath.Dir(g.SocketPath), - g.SocketPath, + runUserDir, ).Run() } -// This function will normalize the location of the forwarded socket. -// the forwarding that happens in pkg/ssh/forward.go will forward the socket in -// the same path (eg. /Users/foo/.gnupg/S.gpg-agent) -// This function will use hardlinks to normalize it to where linux usually -// expects the socket to be. func (g *GPGConf) SetupRemoteSocketLink() error { - err := os.MkdirAll(filepath.Join(os.Getenv("HOME"), ".gnupg"), 0o700) - if err != nil { - return err - } - - err = exec.Command("sudo", "ln", "-s", "-f", g.SocketPath, "/tmp/S.gpg-agent").Run() - if err != nil { - return err - } - - symlinks := []string{ + links := []string{ filepath.Join(os.Getenv("HOME"), ".gnupg", "S.gpg-agent"), - "/run/user/" + strconv.Itoa(os.Getuid()) + "/gnupg/S.gpg-agent", + filepath.Join("/run/user", strconv.Itoa(os.Getuid()), "gnupg", "S.gpg-agent"), } - for _, link := range symlinks { + for _, link := range links { + //nolint:gosec // link derives from $HOME and the current uid, not user input + if err := os.MkdirAll(filepath.Dir(link), 0o700); err != nil { + return err + } _ = os.Remove(link) - _ = os.MkdirAll(filepath.Dir(link), 0o755) - - err = os.Symlink("/tmp/S.gpg-agent", link) - if err != nil { + if err := os.Symlink(g.SocketPath, link); err != nil { return err } } - return nil + return g.claimForwardedSocket() +} + +// claimForwardedSocket takes ownership of the socket, which the ssh server +// binds as root; a non-root user needs write access to connect. It is bound +// asynchronously, so wait briefly for it to appear. +func (g *GPGConf) claimForwardedSocket() error { + owner := strconv.Itoa(os.Getuid()) + ":" + strconv.Itoa(os.Getgid()) + for range 30 { + if _, err := os.Stat(g.SocketPath); err == nil { + //nolint:gosec // g.SocketPath is the fixed forwarded socket path + return exec.Command("sudo", "chown", owner, g.SocketPath).Run() + } + time.Sleep(100 * time.Millisecond) + } + return fmt.Errorf("forwarded gpg socket %q did not appear", g.SocketPath) } -func (g *GPGConf) getConfigPath() string { +func gpgConfigPath() string { return filepath.Join(os.Getenv("HOME"), ".gnupg", "gpg.conf") } diff --git a/pkg/gpg/gpg_forwarding_test.go b/pkg/gpg/gpg_forwarding_test.go new file mode 100644 index 000000000..f3f485291 --- /dev/null +++ b/pkg/gpg/gpg_forwarding_test.go @@ -0,0 +1,74 @@ +package gpg + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestSetupGpgConf_WritesRequiredDirectives(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + require.NoError(t, os.MkdirAll(filepath.Join(home, ".gnupg"), 0o700)) + + require.NoError(t, SetupGpgConf()) + + got := readConf(t, gpgConfigPath()) + for _, d := range gpgConfDirectives { + assert.Contains(t, got, d, "gpg.conf must enable %q for forwarding", d) + } + assert.Contains(t, gpgConfDirectives, "no-autostart") +} + +func TestSetupGpgConf_Idempotent(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + require.NoError(t, os.MkdirAll(filepath.Join(home, ".gnupg"), 0o700)) + + require.NoError(t, SetupGpgConf()) + require.NoError(t, SetupGpgConf()) + + for _, d := range gpgConfDirectives { + assert.Equal(t, 1, strings.Count(readConf(t, gpgConfigPath()), d+"\n"), + "directive %q should not be duplicated on repeated setup", d) + } +} + +func TestSetupGpgConf_PreservesExistingDirectives(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + require.NoError(t, os.MkdirAll(filepath.Join(home, ".gnupg"), 0o700)) + + require.NoError(t, os.WriteFile(gpgConfigPath(), []byte("use-agent\n"), 0o600)) + require.NoError(t, SetupGpgConf()) + + got := readConf(t, gpgConfigPath()) + assert.Equal(t, 1, strings.Count(got, "use-agent\n")) + assert.Contains(t, got, "no-autostart") +} + +func TestSetupGpgConf_ExistingFileWithoutTrailingNewline(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + require.NoError(t, os.MkdirAll(filepath.Join(home, ".gnupg"), 0o700)) + + require.NoError(t, os.WriteFile(gpgConfigPath(), []byte("use-agent"), 0o600)) + require.NoError(t, SetupGpgConf()) + + got := readConf(t, gpgConfigPath()) + assert.NotContains(t, got, "use-agentno-autostart", "directives must not be concatenated") + for _, line := range []string{"use-agent", "no-autostart"} { + assert.True(t, containsDirective(got, line), "missing directive %q in %q", line, got) + } +} + +func readConf(t *testing.T, path string) string { + t.Helper() + b, err := os.ReadFile(path) //nolint:gosec // test path is created by the test + require.NoError(t, err) + return string(b) +} diff --git a/pkg/ide/opener/opener.go b/pkg/ide/opener/opener.go index e7a7ab3cd..ff3a26333 100644 --- a/pkg/ide/opener/opener.go +++ b/pkg/ide/opener/opener.go @@ -324,7 +324,7 @@ type browserIDESpec struct { func openBrowserIDE(ctx context.Context, params IDEParams, spec browserIDESpec) (string, error) { if params.GPGAgentForwarding { - if err := gpg.ForwardAgent(params.Client); err != nil { + if err := gpg.ForwardAgent(ctx, params.Client); err != nil { return "", err } }