From 730f0368d0b2907dcd6001765179f10489bf917f Mon Sep 17 00:00:00 2001 From: Percy Wegmann Date: Fri, 14 Jun 2024 13:28:39 -0500 Subject: [PATCH] ssh/tailssh: replace incubator process with su instead of running su as child This allows the SSH_AUTH_SOCK environment variable to work inside of su and agent forwarding to succeed. Fixes #12467 Signed-off-by: Percy Wegmann --- .github/workflows/ssh-integrationtest.yml | 23 +++ Makefile | 4 +- ssh/tailssh/incubator.go | 16 +- ssh/tailssh/tailssh_integration_test.go | 203 +++++++++++++++++----- ssh/tailssh/testcontainers/Dockerfile | 7 +- 5 files changed, 202 insertions(+), 51 deletions(-) create mode 100644 .github/workflows/ssh-integrationtest.yml diff --git a/.github/workflows/ssh-integrationtest.yml b/.github/workflows/ssh-integrationtest.yml new file mode 100644 index 000000000..c3d0f12e9 --- /dev/null +++ b/.github/workflows/ssh-integrationtest.yml @@ -0,0 +1,23 @@ +# Run the ssh integration tests with `make sshintegrationtest`. +# These tests can also be running locally. +name: "ssh-integrationtest" + +concurrency: + group: ${{ github.workflow }}-$${{ github.head_ref || github.run_id }} + cancel-in-progress: true + +on: + pull_request: + paths: + - "ssh/**" + - "tempfork/gliderlabs/ssh/**" + - ".github/workflows/ssh-integrationtest" +jobs: + ssh-integrationtest: + runs-on: ubuntu-latest + steps: + - name: Check out code + uses: actions/checkout@v4 + - name: Run SSH integration tests + run: | + make sshintegrationtest \ No newline at end of file diff --git a/Makefile b/Makefile index 4ae343c3b..8a62079aa 100644 --- a/Makefile +++ b/Makefile @@ -110,8 +110,8 @@ publishdevnameserver: ## Build and publish k8s-nameserver image to location spec .PHONY: sshintegrationtest sshintegrationtest: ## Run the SSH integration tests in various Docker containers - @GOOS=linux GOARCH=amd64 go test -tags integrationtest -c ./ssh/tailssh -o ssh/tailssh/testcontainers/tailssh.test && \ - GOOS=linux GOARCH=amd64 go build -o ssh/tailssh/testcontainers/tailscaled ./cmd/tailscaled && \ + @GOOS=linux GOARCH=amd64 ./tool/go test -tags integrationtest -c ./ssh/tailssh -o ssh/tailssh/testcontainers/tailssh.test && \ + GOOS=linux GOARCH=amd64 ./tool/go build -o ssh/tailssh/testcontainers/tailscaled ./cmd/tailscaled && \ echo "Testing on ubuntu:focal" && docker build --build-arg="BASE=ubuntu:focal" -t ssh-ubuntu-focal ssh/tailssh/testcontainers && \ echo "Testing on ubuntu:jammy" && docker build --build-arg="BASE=ubuntu:jammy" -t ssh-ubuntu-jammy ssh/tailssh/testcontainers && \ echo "Testing on ubuntu:mantic" && docker build --build-arg="BASE=ubuntu:mantic" -t ssh-ubuntu-mantic ssh/tailssh/testcontainers && \ diff --git a/ssh/tailssh/incubator.go b/ssh/tailssh/incubator.go index 799c116c1..aafe0c743 100644 --- a/ssh/tailssh/incubator.go +++ b/ssh/tailssh/incubator.go @@ -400,8 +400,12 @@ func tryExecLogin(dlogf logger.Logf, ia incubatorArgs) error { } loginArgs := ia.loginArgs(loginCmdPath) dlogf("logging in with %s %+v", loginCmdPath, loginArgs) - // replace the running process - return unix.Exec(loginCmdPath, loginArgs, os.Environ()) + + // If Exec works, the Go code will not proceed past this: + err = unix.Exec(loginCmdPath, loginArgs, os.Environ()) + + // If we made it here, Exec failed. + return err } // trySU attempts to start a login shell using su. If su is available and @@ -438,8 +442,12 @@ func trySU(dlogf logger.Logf, ia incubatorArgs) (handled bool, err error) { } dlogf("logging in with %s %q", su, loginArgs) - cmd := newCommand(ia.hasTTY, su, loginArgs) - return true, cmd.Run() + + // If Exec works, the Go code will not proceed past this: + err = unix.Exec(su, loginArgs, os.Environ()) + + // If we made it here, Exec failed. + return true, err } // findSU attempts to find an su command which supports the -l and -c flags. diff --git a/ssh/tailssh/tailssh_integration_test.go b/ssh/tailssh/tailssh_integration_test.go index 1a2ee91cf..fe317e727 100644 --- a/ssh/tailssh/tailssh_integration_test.go +++ b/ssh/tailssh/tailssh_integration_test.go @@ -8,14 +8,13 @@ package tailssh import ( "bufio" + "bytes" "context" - "crypto/ecdsa" - "crypto/ed25519" - "crypto/elliptic" "crypto/rand" "crypto/rsa" "crypto/x509" "encoding/pem" + "errors" "fmt" "io" "log" @@ -24,6 +23,7 @@ import ( "net/netip" "os" "os/exec" + "path/filepath" "runtime" "strings" "testing" @@ -34,8 +34,10 @@ import ( "github.com/pkg/sftp" gossh "github.com/tailscale/golang-x-crypto/ssh" "golang.org/x/crypto/ssh" + "golang.org/x/crypto/ssh/agent" "tailscale.com/net/tsdial" "tailscale.com/tailcfg" + glider "tailscale.com/tempfork/gliderlabs/ssh" "tailscale.com/types/key" "tailscale.com/types/netmap" "tailscale.com/util/set" @@ -300,6 +302,95 @@ func TestIntegrationSCP(t *testing.T) { } } +func TestSSHAgentForwarding(t *testing.T) { + debugTest.Store(true) + t.Cleanup(func() { + debugTest.Store(false) + }) + + // Create a client SSH key + tmpDir, err := os.MkdirTemp("", "") + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { + _ = os.RemoveAll(tmpDir) + }) + pkFile := filepath.Join(tmpDir, "pk") + clientKey, clientKeyRSA := generateClientKey(t, pkFile) + + // Start upstream SSH server + l, err := net.Listen("tcp", "127.0.0.1:") + if err != nil { + t.Fatalf("unable to listen for SSH: %s", err) + } + t.Cleanup(func() { + _ = l.Close() + }) + + // Run an SSH server that accepts connections from that client SSH key. + gs := glider.Server{ + Handler: func(s glider.Session) { + io.WriteString(s, "Hello world\n") + }, + PublicKeyHandler: func(ctx glider.Context, key glider.PublicKey) error { + // Note - this is not meant to be cryptographically secure, it's + // just checking that SSH agent forwarding is forwarding the right + // key. + a := key.Marshal() + b := clientKey.PublicKey().Marshal() + if !bytes.Equal(a, b) { + return errors.New("key mismatch") + } + return nil + }, + } + go gs.Serve(l) + + // Run tailscale SSH server and connect to it + username := "testuser" + tailscaleAddr := testServer(t, username, false) // TODO: make this false to use V2 behavior + tcl, err := ssh.Dial("tcp", tailscaleAddr, &ssh.ClientConfig{ + HostKeyCallback: ssh.InsecureIgnoreHostKey(), + }) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { tcl.Close() }) + + s, err := tcl.NewSession() + if err != nil { + t.Fatal(err) + } + + // Set up SSH agent forwarding on the client + err = agent.RequestAgentForwarding(s) + if err != nil { + t.Fatal(err) + } + + keyring := agent.NewKeyring() + keyring.Add(agent.AddedKey{ + PrivateKey: clientKeyRSA, + }) + err = agent.ForwardToAgent(tcl, keyring) + if err != nil { + t.Fatal(err) + } + + // Attempt to SSH to the upstream test server using the forwarded SSH key + // and run the "true" command. + upstreamHost, upstreamPort, err := net.SplitHostPort(l.Addr().String()) + if err != nil { + t.Fatal(err) + } + + o, err := s.CombinedOutput(fmt.Sprintf(`ssh -T -o StrictHostKeyChecking=no -p %s upstreamuser@%s "true"`, upstreamPort, upstreamHost)) + if err != nil { + t.Fatalf("unable to call true command: %s\n%s", err, o) + } +} + func fallbackToSUAvailable() bool { if runtime.GOOS != "linux" { return false @@ -374,10 +465,25 @@ readLoop: return string(_got) } -func testClient(t *testing.T, forceV1Behavior bool) *ssh.Client { +func testClient(t *testing.T, forceV1Behavior bool, authMethods ...ssh.AuthMethod) *ssh.Client { t.Helper() username := "testuser" + addr := testServer(t, username, forceV1Behavior) + + cl, err := ssh.Dial("tcp", addr, &ssh.ClientConfig{ + HostKeyCallback: ssh.InsecureIgnoreHostKey(), + Auth: authMethods, + }) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { cl.Close() }) + + return cl +} + +func testServer(t *testing.T, username string, forceV1Behavior bool) string { srv := &server{ lb: &testBackend{localUser: username, forceV1Behavior: forceV1Behavior}, logf: log.Printf, @@ -392,21 +498,15 @@ func testClient(t *testing.T, forceV1Behavior bool) *ssh.Client { t.Cleanup(func() { l.Close() }) go func() { - conn, err := l.Accept() - if err == nil { - go srv.HandleSSHConn(&addressFakingConn{conn}) + for { + conn, err := l.Accept() + if err == nil { + go srv.HandleSSHConn(&addressFakingConn{conn}) + } } }() - cl, err := ssh.Dial("tcp", l.Addr().String(), &ssh.ClientConfig{ - HostKeyCallback: ssh.InsecureIgnoreHostKey(), - }) - if err != nil { - log.Fatal(err) - } - t.Cleanup(func() { cl.Close() }) - - return cl + return l.Addr().String() } func testSession(t *testing.T, forceV1Behavior bool) *session { @@ -417,7 +517,7 @@ func testSession(t *testing.T, forceV1Behavior bool) *session { func testSessionFor(t *testing.T, cl *ssh.Client) *session { s, err := cl.NewSession() if err != nil { - log.Fatal(err) + t.Fatal(err) } t.Cleanup(func() { s.Close() }) @@ -435,6 +535,31 @@ func testSessionFor(t *testing.T, cl *ssh.Client) *session { } } +func generateClientKey(t *testing.T, privateKeyFile string) (ssh.Signer, *rsa.PrivateKey) { + t.Helper() + priv, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + t.Fatal(err) + } + mk, err := x509.MarshalPKCS8PrivateKey(priv) + if err != nil { + t.Fatal(err) + } + privateKey := pem.EncodeToMemory(&pem.Block{Type: "PRIVATE KEY", Bytes: mk}) + if privateKey == nil { + t.Fatal("failed to encoded private key") + } + err = os.WriteFile(privateKeyFile, privateKey, 0600) + if err != nil { + t.Fatal(err) + } + signer, err := ssh.ParsePrivateKey(privateKey) + if err != nil { + t.Fatal(err) + } + return signer, priv +} + // testBackend implements ipnLocalBackend type testBackend struct { localUser string @@ -443,33 +568,23 @@ type testBackend struct { func (tb *testBackend) GetSSH_HostKeys() ([]gossh.Signer, error) { var result []gossh.Signer - for _, typ := range []string{"ed25519", "ecdsa", "rsa"} { - var priv any - var err error - switch typ { - case "ed25519": - _, priv, err = ed25519.GenerateKey(rand.Reader) - case "ecdsa": - curve := elliptic.P256() - priv, err = ecdsa.GenerateKey(curve, rand.Reader) - case "rsa": - const keySize = 2048 - priv, err = rsa.GenerateKey(rand.Reader, keySize) - } - if err != nil { - return nil, err - } - mk, err := x509.MarshalPKCS8PrivateKey(priv) - if err != nil { - return nil, err - } - hostKey := pem.EncodeToMemory(&pem.Block{Type: "PRIVATE KEY", Bytes: mk}) - signer, err := gossh.ParsePrivateKey(hostKey) - if err != nil { - return nil, err - } - result = append(result, signer) + var priv any + var err error + const keySize = 2048 + priv, err = rsa.GenerateKey(rand.Reader, keySize) + if err != nil { + return nil, err + } + mk, err := x509.MarshalPKCS8PrivateKey(priv) + if err != nil { + return nil, err + } + hostKey := pem.EncodeToMemory(&pem.Block{Type: "PRIVATE KEY", Bytes: mk}) + signer, err := gossh.ParsePrivateKey(hostKey) + if err != nil { + return nil, err } + result = append(result, signer) return result, nil } @@ -487,7 +602,7 @@ func (tb *testBackend) NetMap() *netmap.NetworkMap { Rules: []*tailcfg.SSHRule{ { Principals: []*tailcfg.SSHPrincipal{{Any: true}}, - Action: &tailcfg.SSHAction{Accept: true}, + Action: &tailcfg.SSHAction{Accept: true, AllowAgentForwarding: true}, SSHUsers: map[string]string{"*": tb.localUser}, }, }, diff --git a/ssh/tailssh/testcontainers/Dockerfile b/ssh/tailssh/testcontainers/Dockerfile index d712dd298..ff27981ef 100644 --- a/ssh/tailssh/testcontainers/Dockerfile +++ b/ssh/tailssh/testcontainers/Dockerfile @@ -20,7 +20,8 @@ COPY tailssh.test . RUN chmod 755 tailscaled -RUN echo "First run tests normally." +# RUN echo "First run tests normally." +RUN eval `ssh-agent -s` && TAILSCALED_PATH=`pwd`tailscaled ./tailssh.test -test.v -test.run TestSSHAgentForwarding RUN rm -Rf /home/testuser RUN TAILSCALED_PATH=`pwd`tailscaled ./tailssh.test -test.v -test.run TestIntegrationSFTP RUN rm -Rf /home/testuser @@ -30,6 +31,7 @@ RUN TAILSCALED_PATH=`pwd`tailscaled ./tailssh.test -test.v -test.run TestIntegra RUN echo "Then run tests as non-root user testuser and make sure tests still pass." RUN chown testuser:groupone /tmp/tailscalessh.log +RUN TAILSCALED_PATH=`pwd`tailscaled eval `su -m testuser -c ssh-agent -s` && su -m testuser -c "./tailssh.test -test.v -test.run TestSSHAgentForwarding" RUN TAILSCALED_PATH=`pwd`tailscaled su -m testuser -c "./tailssh.test -test.v -test.run TestIntegration TestDoDropPrivileges" RUN chown root:root /tmp/tailscalessh.log @@ -42,12 +44,14 @@ RUN chmod 755 /usr/bin/login # Simulate getenforce command RUN printf "#!/bin/bash\necho 'Enforcing'" > /usr/bin/getenforce RUN chmod 755 /usr/bin/getenforce +RUN eval `ssh-agent -s` && TAILSCALED_PATH=`pwd`tailscaled ./tailssh.test -test.v -test.run TestSSHAgentForwarding RUN TAILSCALED_PATH=`pwd`tailscaled ./tailssh.test -test.v -test.run TestIntegration RUN mv /tmp/login_orig /usr/bin/login RUN rm /usr/bin/getenforce RUN echo "Then remove the login command and make sure tests still pass." RUN rm `which login` +RUN eval `ssh-agent -s` && TAILSCALED_PATH=`pwd`tailscaled ./tailssh.test -test.v -test.run TestSSHAgentForwarding RUN rm -Rf /home/testuser RUN TAILSCALED_PATH=`pwd`tailscaled ./tailssh.test -test.v -test.run TestIntegrationSFTP RUN rm -Rf /home/testuser @@ -58,6 +62,7 @@ RUN TAILSCALED_PATH=`pwd`tailscaled ./tailssh.test -test.v -test.run TestIntegra RUN echo "Then remove the su command and make sure tests still pass." RUN chown root:root /tmp/tailscalessh.log RUN rm `which su` +RUN eval `ssh-agent -s` && TAILSCALED_PATH=`pwd`tailscaled ./tailssh.test -test.v -test.run TestSSHAgentForwarding RUN TAILSCALED_PATH=`pwd`tailscaled ./tailssh.test -test.v -test.run TestIntegration RUN echo "Test doDropPrivileges"