From b60f6b849af1fae1cf343be98f7fb1714c9ea165 Mon Sep 17 00:00:00 2001 From: Percy Wegmann Date: Wed, 29 Jan 2025 10:25:50 -0600 Subject: [PATCH] Revert "ssh,tempfork/gliderlabs/ssh: replace github.com/tailscale/golang-x-crypto/ssh with golang.org/x/crypto/ssh" This reverts commit 46fd4e58a27495263336b86ee961ee28d8c332b7. We don't want to include this in 1.80 yet, but can add it back post 1.80. Updates #8593 Signed-off-by: Percy Wegmann --- cmd/k8s-operator/depaware.txt | 11 +- cmd/ssh-auth-none-demo/ssh-auth-none-demo.go | 24 +- cmd/tailscaled/depaware.txt | 7 +- cmd/tailscaled/deps_test.go | 1 + go.mod | 2 +- go.sum | 4 +- ipn/ipnlocal/ssh.go | 2 +- ssh/tailssh/tailssh.go | 316 +++++++++++-------- ssh/tailssh/tailssh_integration_test.go | 2 +- ssh/tailssh/tailssh_test.go | 5 +- tempfork/gliderlabs/ssh/agent.go | 2 +- tempfork/gliderlabs/ssh/context.go | 11 +- tempfork/gliderlabs/ssh/options.go | 2 +- tempfork/gliderlabs/ssh/options_test.go | 2 +- tempfork/gliderlabs/ssh/server.go | 2 +- tempfork/gliderlabs/ssh/session.go | 2 +- tempfork/gliderlabs/ssh/session_test.go | 2 +- tempfork/gliderlabs/ssh/ssh.go | 4 +- tempfork/gliderlabs/ssh/tcpip.go | 2 +- tempfork/gliderlabs/ssh/tcpip_test.go | 2 +- tempfork/gliderlabs/ssh/util.go | 2 +- tempfork/gliderlabs/ssh/wrap.go | 2 +- 22 files changed, 234 insertions(+), 175 deletions(-) diff --git a/cmd/k8s-operator/depaware.txt b/cmd/k8s-operator/depaware.txt index 972dbfc2c..e32fd4a2b 100644 --- a/cmd/k8s-operator/depaware.txt +++ b/cmd/k8s-operator/depaware.txt @@ -197,6 +197,9 @@ tailscale.com/cmd/k8s-operator dependencies: (generated by github.com/tailscale/ W 💣 github.com/tailscale/go-winio/internal/socket from github.com/tailscale/go-winio W github.com/tailscale/go-winio/internal/stringbuffer from github.com/tailscale/go-winio/internal/fs W github.com/tailscale/go-winio/pkg/guid from github.com/tailscale/go-winio+ + LD github.com/tailscale/golang-x-crypto/internal/poly1305 from github.com/tailscale/golang-x-crypto/ssh + LD github.com/tailscale/golang-x-crypto/ssh from tailscale.com/ipn/ipnlocal + LD github.com/tailscale/golang-x-crypto/ssh/internal/bcrypt_pbkdf from github.com/tailscale/golang-x-crypto/ssh github.com/tailscale/goupnp from github.com/tailscale/goupnp/dcps/internetgateway2+ github.com/tailscale/goupnp/dcps/internetgateway2 from tailscale.com/net/portmapper github.com/tailscale/goupnp/httpu from github.com/tailscale/goupnp+ @@ -983,12 +986,12 @@ tailscale.com/cmd/k8s-operator dependencies: (generated by github.com/tailscale/ golang.org/x/crypto/argon2 from tailscale.com/tka golang.org/x/crypto/blake2b from golang.org/x/crypto/argon2+ golang.org/x/crypto/blake2s from github.com/tailscale/wireguard-go/device+ - LD golang.org/x/crypto/blowfish from golang.org/x/crypto/ssh/internal/bcrypt_pbkdf - golang.org/x/crypto/chacha20 from golang.org/x/crypto/ssh+ + LD golang.org/x/crypto/blowfish from github.com/tailscale/golang-x-crypto/ssh/internal/bcrypt_pbkdf + golang.org/x/crypto/chacha20 from github.com/tailscale/golang-x-crypto/ssh+ golang.org/x/crypto/chacha20poly1305 from crypto/tls+ golang.org/x/crypto/cryptobyte from crypto/ecdsa+ golang.org/x/crypto/cryptobyte/asn1 from crypto/ecdsa+ - golang.org/x/crypto/curve25519 from golang.org/x/crypto/ssh+ + golang.org/x/crypto/curve25519 from github.com/tailscale/golang-x-crypto/ssh+ golang.org/x/crypto/hkdf from crypto/tls+ golang.org/x/crypto/internal/alias from golang.org/x/crypto/chacha20+ golang.org/x/crypto/internal/poly1305 from golang.org/x/crypto/chacha20poly1305+ @@ -997,8 +1000,6 @@ tailscale.com/cmd/k8s-operator dependencies: (generated by github.com/tailscale/ golang.org/x/crypto/poly1305 from github.com/tailscale/wireguard-go/device golang.org/x/crypto/salsa20/salsa from golang.org/x/crypto/nacl/box+ golang.org/x/crypto/sha3 from crypto/internal/mlkem768+ - LD golang.org/x/crypto/ssh from tailscale.com/ipn/ipnlocal - LD golang.org/x/crypto/ssh/internal/bcrypt_pbkdf from golang.org/x/crypto/ssh golang.org/x/exp/constraints from github.com/dblohm7/wingoes/pe+ golang.org/x/exp/maps from sigs.k8s.io/controller-runtime/pkg/cache+ golang.org/x/exp/slices from tailscale.com/cmd/k8s-operator+ diff --git a/cmd/ssh-auth-none-demo/ssh-auth-none-demo.go b/cmd/ssh-auth-none-demo/ssh-auth-none-demo.go index 39af584ec..ee929299a 100644 --- a/cmd/ssh-auth-none-demo/ssh-auth-none-demo.go +++ b/cmd/ssh-auth-none-demo/ssh-auth-none-demo.go @@ -6,9 +6,6 @@ // highlight the unique parts of the Tailscale SSH server so SSH // client authors can hit it easily and fix their SSH clients without // needing to set up Tailscale and Tailscale SSH. -// -// Connections are allowed using any username except for "denyme". Connecting as -// "denyme" will result in an authentication failure with error message. package main import ( @@ -19,7 +16,6 @@ "crypto/rsa" "crypto/x509" "encoding/pem" - "errors" "flag" "fmt" "io" @@ -28,7 +24,7 @@ "path/filepath" "time" - gossh "golang.org/x/crypto/ssh" + gossh "github.com/tailscale/golang-x-crypto/ssh" "tailscale.com/tempfork/gliderlabs/ssh" ) @@ -66,21 +62,13 @@ func main() { Handler: handleSessionPostSSHAuth, ServerConfigCallback: func(ctx ssh.Context) *gossh.ServerConfig { start := time.Now() - var spac gossh.ServerPreAuthConn return &gossh.ServerConfig{ - PreAuthConnCallback: func(conn gossh.ServerPreAuthConn) { - spac = conn + NextAuthMethodCallback: func(conn gossh.ConnMetadata, prevErrors []error) []string { + return []string{"tailscale"} }, NoClientAuth: true, // required for the NoClientAuthCallback to run NoClientAuthCallback: func(cm gossh.ConnMetadata) (*gossh.Permissions, error) { - spac.SendAuthBanner(fmt.Sprintf("# Banner: doing none auth at %v\r\n", time.Since(start))) - - if cm.User() == "denyme" { - return nil, &gossh.BannerError{ - Err: errors.New("denying access"), - Message: "denyme is not allowed to access this machine\n", - } - } + cm.SendAuthBanner(fmt.Sprintf("# Banner: doing none auth at %v\r\n", time.Since(start))) totalBanners := 2 if cm.User() == "banners" { @@ -89,9 +77,9 @@ func main() { for banner := 2; banner <= totalBanners; banner++ { time.Sleep(time.Second) if banner == totalBanners { - spac.SendAuthBanner(fmt.Sprintf("# Banner%d: access granted at %v\r\n", banner, time.Since(start))) + cm.SendAuthBanner(fmt.Sprintf("# Banner%d: access granted at %v\r\n", banner, time.Since(start))) } else { - spac.SendAuthBanner(fmt.Sprintf("# Banner%d at %v\r\n", banner, time.Since(start))) + cm.SendAuthBanner(fmt.Sprintf("# Banner%d at %v\r\n", banner, time.Since(start))) } } return nil, nil diff --git a/cmd/tailscaled/depaware.txt b/cmd/tailscaled/depaware.txt index a6fae54ff..a7ad83818 100644 --- a/cmd/tailscaled/depaware.txt +++ b/cmd/tailscaled/depaware.txt @@ -152,6 +152,9 @@ tailscale.com/cmd/tailscaled dependencies: (generated by github.com/tailscale/de W 💣 github.com/tailscale/go-winio/internal/socket from github.com/tailscale/go-winio W github.com/tailscale/go-winio/internal/stringbuffer from github.com/tailscale/go-winio/internal/fs W github.com/tailscale/go-winio/pkg/guid from github.com/tailscale/go-winio+ + LD github.com/tailscale/golang-x-crypto/internal/poly1305 from github.com/tailscale/golang-x-crypto/ssh + LD github.com/tailscale/golang-x-crypto/ssh from tailscale.com/ipn/ipnlocal+ + LD github.com/tailscale/golang-x-crypto/ssh/internal/bcrypt_pbkdf from github.com/tailscale/golang-x-crypto/ssh github.com/tailscale/goupnp from github.com/tailscale/goupnp/dcps/internetgateway2+ github.com/tailscale/goupnp/dcps/internetgateway2 from tailscale.com/net/portmapper github.com/tailscale/goupnp/httpu from github.com/tailscale/goupnp+ @@ -436,12 +439,12 @@ tailscale.com/cmd/tailscaled dependencies: (generated by github.com/tailscale/de golang.org/x/crypto/argon2 from tailscale.com/tka golang.org/x/crypto/blake2b from golang.org/x/crypto/argon2+ golang.org/x/crypto/blake2s from github.com/tailscale/wireguard-go/device+ - LD golang.org/x/crypto/blowfish from golang.org/x/crypto/ssh/internal/bcrypt_pbkdf + LD golang.org/x/crypto/blowfish from github.com/tailscale/golang-x-crypto/ssh/internal/bcrypt_pbkdf+ golang.org/x/crypto/chacha20 from golang.org/x/crypto/chacha20poly1305+ golang.org/x/crypto/chacha20poly1305 from crypto/tls+ golang.org/x/crypto/cryptobyte from crypto/ecdsa+ golang.org/x/crypto/cryptobyte/asn1 from crypto/ecdsa+ - golang.org/x/crypto/curve25519 from golang.org/x/crypto/ssh+ + golang.org/x/crypto/curve25519 from github.com/tailscale/golang-x-crypto/ssh+ golang.org/x/crypto/hkdf from crypto/tls+ golang.org/x/crypto/internal/alias from golang.org/x/crypto/chacha20+ golang.org/x/crypto/internal/poly1305 from golang.org/x/crypto/chacha20poly1305+ diff --git a/cmd/tailscaled/deps_test.go b/cmd/tailscaled/deps_test.go index 7f06abc6c..2b4bc280d 100644 --- a/cmd/tailscaled/deps_test.go +++ b/cmd/tailscaled/deps_test.go @@ -17,6 +17,7 @@ func TestOmitSSH(t *testing.T) { Tags: "ts_omit_ssh", BadDeps: map[string]string{ "tailscale.com/ssh/tailssh": msg, + "golang.org/x/crypto/ssh": msg, "tailscale.com/sessionrecording": msg, "github.com/anmitsu/go-shlex": msg, "github.com/creack/pty": msg, diff --git a/go.mod b/go.mod index 2489e34d7..8e52a9ab3 100644 --- a/go.mod +++ b/go.mod @@ -94,7 +94,7 @@ require ( go.uber.org/zap v1.27.0 go4.org/mem v0.0.0-20240501181205-ae6ca9944745 go4.org/netipx v0.0.0-20231129151722-fdeea329fbba - golang.org/x/crypto v0.32.1-0.20250118192723-a8ea4be81f07 + golang.org/x/crypto v0.32.0 golang.org/x/exp v0.0.0-20250106191152-7588d65b2ba8 golang.org/x/mod v0.22.0 golang.org/x/net v0.34.0 diff --git a/go.sum b/go.sum index b10e98da2..c1c82ad77 100644 --- a/go.sum +++ b/go.sum @@ -1058,8 +1058,8 @@ golang.org/x/crypto v0.0.0-20210921155107-089bfa567519/go.mod h1:GvvjBRRGRdwPK5y golang.org/x/crypto v0.0.0-20220622213112-05595931fe9d/go.mod h1:IxCIyHEi3zRg3s0A5j5BB6A9Jmi73HwBIUl50j+osU4= golang.org/x/crypto v0.1.0/go.mod h1:RecgLatLF4+eUMCP1PoPZQb+cVrJcOPbHkTkbkB9sbw= golang.org/x/crypto v0.3.0/go.mod h1:hebNnKkNXi2UzZN1eVRvBB7co0a+JxK6XbPiWVs/3J4= -golang.org/x/crypto v0.32.1-0.20250118192723-a8ea4be81f07 h1:Z+Zg+aXJYq6f4TK2E4H+vZkQ4dJAWnInXDR6hM9znxo= -golang.org/x/crypto v0.32.1-0.20250118192723-a8ea4be81f07/go.mod h1:ZnnJkOaASj8g0AjIduWNlq2NRxL0PlBrbKVyZ6V/Ugc= +golang.org/x/crypto v0.32.0 h1:euUpcYgM8WcP71gNpTqQCn6rC2t6ULUPiOzfWaXVVfc= +golang.org/x/crypto v0.32.0/go.mod h1:ZnnJkOaASj8g0AjIduWNlq2NRxL0PlBrbKVyZ6V/Ugc= golang.org/x/exp v0.0.0-20190121172915-509febef88a4/go.mod h1:CJ0aWSM057203Lf6IL+f9T1iT9GByDxfZKAQTCR3kQA= golang.org/x/exp v0.0.0-20190306152737-a1d7652674e8/go.mod h1:CJ0aWSM057203Lf6IL+f9T1iT9GByDxfZKAQTCR3kQA= golang.org/x/exp v0.0.0-20190510132918-efd6b22b2522/go.mod h1:ZjyILWgesfNpC6sMxTJOJm9Kp84zZh5NQWvqDGG3Qr8= diff --git a/ipn/ipnlocal/ssh.go b/ipn/ipnlocal/ssh.go index 47a74e282..383d03f5a 100644 --- a/ipn/ipnlocal/ssh.go +++ b/ipn/ipnlocal/ssh.go @@ -24,8 +24,8 @@ "strings" "sync" + "github.com/tailscale/golang-x-crypto/ssh" "go4.org/mem" - "golang.org/x/crypto/ssh" "tailscale.com/tailcfg" "tailscale.com/util/lineiter" "tailscale.com/util/mak" diff --git a/ssh/tailssh/tailssh.go b/ssh/tailssh/tailssh.go index 638ff99b8..7f21ccd11 100644 --- a/ssh/tailssh/tailssh.go +++ b/ssh/tailssh/tailssh.go @@ -29,7 +29,7 @@ "syscall" "time" - gossh "golang.org/x/crypto/ssh" + gossh "github.com/tailscale/golang-x-crypto/ssh" "tailscale.com/envknob" "tailscale.com/ipn/ipnlocal" "tailscale.com/logtail/backoff" @@ -198,11 +198,8 @@ func (srv *server) OnPolicyChange() { // Setup and discover server info // - ServerConfigCallback // -// Get access to a ServerPreAuthConn (useful for sending banners) -// -// Do the user auth with a NoClientAuthCallback. If user specified -// a username ending in "+password", follow this with password auth -// (to work around buggy SSH clients that don't work with noauth). +// Do the user auth +// - NoClientAuthHandler // // Once auth is done, the conn can be multiplexed with multiple sessions and // channels concurrently. At which point any of the following can be called @@ -222,12 +219,15 @@ type conn struct { idH string connID string // ID that's shared with control - // spac is a [gossh.ServerPreAuthConn] used for sending auth banners. - // Banners cannot be sent after auth completes. - spac gossh.ServerPreAuthConn + // anyPasswordIsOkay is whether the client is authorized but has requested + // password-based auth to work around their buggy SSH client. When set, we + // accept any password in the PasswordHandler. + anyPasswordIsOkay bool // set by NoClientAuthCallback - action0 *tailcfg.SSHAction // set by clientAuth - finalAction *tailcfg.SSHAction // set by clientAuth + action0 *tailcfg.SSHAction // set by doPolicyAuth; first matching action + currentAction *tailcfg.SSHAction // set by doPolicyAuth, updated by resolveNextAction + finalAction *tailcfg.SSHAction // set by doPolicyAuth or resolveNextAction + finalActionErr error // set by doPolicyAuth or resolveNextAction info *sshConnInfo // set by setInfo localUser *userMeta // set by doPolicyAuth @@ -254,142 +254,141 @@ func (c *conn) vlogf(format string, args ...any) { } } +// isAuthorized walks through the action chain and returns nil if the connection +// is authorized. If the connection is not authorized, it returns +// errDenied. If the action chain resolution fails, it returns the +// resolution error. +func (c *conn) isAuthorized(ctx ssh.Context) error { + action := c.currentAction + for { + if action.Accept { + return nil + } + if action.Reject || action.HoldAndDelegate == "" { + return errDenied + } + var err error + action, err = c.resolveNextAction(ctx) + if err != nil { + return err + } + if action.Message != "" { + if err := ctx.SendAuthBanner(action.Message); err != nil { + return err + } + } + } +} + // errDenied is returned by auth callbacks when a connection is denied by the -// policy. It returns a gossh.BannerError to make sure the message gets -// displayed as an auth banner. -func errDenied(message string) error { - if message == "" { - message = "tailscale: access denied" - } - return &gossh.BannerError{ - Message: message, - } -} +// policy. +var errDenied = errors.New("ssh: access denied") -// bannerError creates a gossh.BannerError that will result in the given -// message being displayed to the client. If err != nil, this also logs -// message:error. The contents of err is not leaked to clients in the banner. -func (c *conn) bannerError(message string, err error) error { - if err != nil { - c.logf("%s: %s", message, err) - } - return &gossh.BannerError{ - Err: err, - Message: fmt.Sprintf("tailscale: %s", message), - } -} - -// clientAuth is responsible for performing client authentication. +// NoClientAuthCallback implements gossh.NoClientAuthCallback and is called by +// the ssh.Server when the client first connects with the "none" +// authentication method. // -// If policy evaluation fails, it returns an error. -// If access is denied, it returns an error. -func (c *conn) clientAuth(cm gossh.ConnMetadata) (*gossh.Permissions, error) { +// It is responsible for continuing policy evaluation from BannerCallback (or +// starting it afresh). It returns an error if the policy evaluation fails, or +// if the decision is "reject" +// +// It either returns nil (accept) or errDenied (reject). The errors may be wrapped. +func (c *conn) NoClientAuthCallback(ctx ssh.Context) error { if c.insecureSkipTailscaleAuth { - return &gossh.Permissions{}, nil + return nil + } + if err := c.doPolicyAuth(ctx); err != nil { + return err + } + if err := c.isAuthorized(ctx); err != nil { + return err } - if err := c.setInfo(cm); err != nil { - return nil, c.bannerError("failed to get connection info", err) + // Let users specify a username ending in +password to force password auth. + // This exists for buggy SSH clients that get confused by success from + // "none" auth. + if strings.HasSuffix(ctx.User(), forcePasswordSuffix) { + c.anyPasswordIsOkay = true + return errors.New("any password please") // not shown to users + } + return nil +} + +func (c *conn) nextAuthMethodCallback(cm gossh.ConnMetadata, prevErrors []error) (nextMethod []string) { + switch { + case c.anyPasswordIsOkay: + nextMethod = append(nextMethod, "password") } - action, localUser, acceptEnv, err := c.evaluatePolicy() + // The fake "tailscale" method is always appended to next so OpenSSH renders + // that in parens as the final failure. (It also shows up in "ssh -v", etc) + nextMethod = append(nextMethod, "tailscale") + return +} + +// fakePasswordHandler is our implementation of the PasswordHandler hook that +// checks whether the user's password is correct. But we don't actually use +// passwords. This exists only for when the user's username ends in "+password" +// to signal that their SSH client is buggy and gets confused by auth type +// "none" succeeding and they want our SSH server to require a dummy password +// prompt instead. We then accept any password since we've already authenticated +// & authorized them. +func (c *conn) fakePasswordHandler(ctx ssh.Context, password string) bool { + return c.anyPasswordIsOkay +} + +// doPolicyAuth verifies that conn can proceed. +// It returns nil if the matching policy action is Accept or +// HoldAndDelegate. Otherwise, it returns errDenied. +func (c *conn) doPolicyAuth(ctx ssh.Context) error { + if err := c.setInfo(ctx); err != nil { + c.logf("failed to get conninfo: %v", err) + return errDenied + } + a, localUser, acceptEnv, err := c.evaluatePolicy() if err != nil { - return nil, c.bannerError("failed to evaluate SSH policy", err) + return fmt.Errorf("%w: %v", errDenied, err) } - - c.action0 = action - - if action.Accept || action.HoldAndDelegate != "" { - // Immediately look up user information for purposes of generating - // hold and delegate URL (if necessary). + c.action0 = a + c.currentAction = a + c.acceptEnv = acceptEnv + if a.Message != "" { + if err := ctx.SendAuthBanner(a.Message); err != nil { + return fmt.Errorf("SendBanner: %w", err) + } + } + if a.Accept || a.HoldAndDelegate != "" { + if a.Accept { + c.finalAction = a + } lu, err := userLookup(localUser) if err != nil { - return nil, c.bannerError(fmt.Sprintf("failed to look up local user %q ", localUser), err) + c.logf("failed to look up %v: %v", localUser, err) + ctx.SendAuthBanner(fmt.Sprintf("failed to look up %v\r\n", localUser)) + return err } gids, err := lu.GroupIds() if err != nil { - return nil, c.bannerError("failed to look up local user's group IDs", err) + c.logf("failed to look up local user's group IDs: %v", err) + return err } c.userGroupIDs = gids c.localUser = lu - c.acceptEnv = acceptEnv + return nil } - - for { - switch { - case action.Accept: - metricTerminalAccept.Add(1) - if action.Message != "" { - if err := c.spac.SendAuthBanner(action.Message); err != nil { - return nil, fmt.Errorf("error sending auth welcome message: %w", err) - } - } - c.finalAction = action - return &gossh.Permissions{}, nil - case action.Reject: - metricTerminalReject.Add(1) - c.finalAction = action - return nil, errDenied(action.Message) - case action.HoldAndDelegate != "": - if action.Message != "" { - if err := c.spac.SendAuthBanner(action.Message); err != nil { - return nil, fmt.Errorf("error sending hold and delegate message: %w", err) - } - } - - url := action.HoldAndDelegate - - ctx, cancel := context.WithTimeout(context.Background(), 30*time.Minute) - defer cancel() - - metricHolds.Add(1) - url = c.expandDelegateURLLocked(url) - - var err error - action, err = c.fetchSSHAction(ctx, url) - if err != nil { - metricTerminalFetchError.Add(1) - return nil, c.bannerError("failed to fetch next SSH action", fmt.Errorf("fetch failed from %s: %w", url, err)) - } - default: - metricTerminalMalformed.Add(1) - return nil, c.bannerError("reached Action that had neither Accept, Reject, nor HoldAndDelegate", nil) - } + if a.Reject { + c.finalAction = a + return errDenied } + // Shouldn't get here, but: + return errDenied } // ServerConfig implements ssh.ServerConfigCallback. func (c *conn) ServerConfig(ctx ssh.Context) *gossh.ServerConfig { return &gossh.ServerConfig{ - PreAuthConnCallback: func(spac gossh.ServerPreAuthConn) { - c.spac = spac - }, - NoClientAuth: true, // required for the NoClientAuthCallback to run - NoClientAuthCallback: func(cm gossh.ConnMetadata) (*gossh.Permissions, error) { - // First perform client authentication, which can potentially - // involve multiple steps (for example prompting user to log in to - // Tailscale admin panel to confirm identity). - perms, err := c.clientAuth(cm) - if err != nil { - return nil, err - } - - // Authentication succeeded. Buggy SSH clients get confused by - // success from the "none" auth method. As a workaround, let users - // specify a username ending in "+password" to force password auth. - // The actual value of the password doesn't matter. - if strings.HasSuffix(cm.User(), forcePasswordSuffix) { - return nil, &gossh.PartialSuccessError{ - Next: gossh.ServerAuthCallbacks{ - PasswordCallback: func(_ gossh.ConnMetadata, password []byte) (*gossh.Permissions, error) { - return &gossh.Permissions{}, nil - }, - }, - } - } - - return perms, nil - }, + NoClientAuth: true, // required for the NoClientAuthCallback to run + NextAuthMethodCallback: c.nextAuthMethodCallback, } } @@ -400,7 +399,7 @@ func (srv *server) newConn() (*conn, error) { // Stop accepting new connections. // Connections in the auth phase are handled in handleConnPostSSHAuth. // Existing sessions are terminated by Shutdown. - return nil, errDenied("tailscale: server is shutting down") + return nil, errDenied } srv.mu.Unlock() c := &conn{srv: srv} @@ -411,6 +410,9 @@ func (srv *server) newConn() (*conn, error) { Version: "Tailscale", ServerConfigCallback: c.ServerConfig, + NoClientAuthHandler: c.NoClientAuthCallback, + PasswordHandler: c.fakePasswordHandler, + Handler: c.handleSessionPostSSHAuth, LocalPortForwardingCallback: c.mayForwardLocalPortTo, ReversePortForwardingCallback: c.mayReversePortForwardTo, @@ -521,16 +523,16 @@ func toIPPort(a net.Addr) (ipp netip.AddrPort) { return netip.AddrPortFrom(tanetaddr.Unmap(), uint16(ta.Port)) } -// connInfo populates the sshConnInfo from the provided arguments, +// connInfo returns a populated sshConnInfo from the provided arguments, // validating only that they represent a known Tailscale identity. -func (c *conn) setInfo(cm gossh.ConnMetadata) error { +func (c *conn) setInfo(ctx ssh.Context) error { if c.info != nil { return nil } ci := &sshConnInfo{ - sshUser: strings.TrimSuffix(cm.User(), forcePasswordSuffix), - src: toIPPort(cm.RemoteAddr()), - dst: toIPPort(cm.LocalAddr()), + sshUser: strings.TrimSuffix(ctx.User(), forcePasswordSuffix), + src: toIPPort(ctx.RemoteAddr()), + dst: toIPPort(ctx.LocalAddr()), } if !tsaddr.IsTailscaleIP(ci.dst.Addr()) { return fmt.Errorf("tailssh: rejecting non-Tailscale local address %v", ci.dst) @@ -545,7 +547,7 @@ func (c *conn) setInfo(cm gossh.ConnMetadata) error { ci.node = node ci.uprof = uprof - c.idH = string(cm.SessionID()) + c.idH = ctx.SessionID() c.info = ci c.logf("handling conn: %v", ci.String()) return nil @@ -592,6 +594,62 @@ func (c *conn) handleSessionPostSSHAuth(s ssh.Session) { ss.run() } +// resolveNextAction starts at c.currentAction and makes it way through the +// action chain one step at a time. An action without a HoldAndDelegate is +// considered the final action. Once a final action is reached, this function +// will keep returning that action. It updates c.currentAction to the next +// action in the chain. When the final action is reached, it also sets +// c.finalAction to the final action. +func (c *conn) resolveNextAction(sctx ssh.Context) (action *tailcfg.SSHAction, err error) { + if c.finalAction != nil || c.finalActionErr != nil { + return c.finalAction, c.finalActionErr + } + + defer func() { + if action != nil { + c.currentAction = action + if action.Accept || action.Reject { + c.finalAction = action + } + } + if err != nil { + c.finalActionErr = err + } + }() + + ctx, cancel := context.WithCancel(sctx) + defer cancel() + + // Loop processing/fetching Actions until one reaches a + // terminal state (Accept, Reject, or invalid Action), or + // until fetchSSHAction times out due to the context being + // done (client disconnect) or its 30 minute timeout passes. + // (Which is a long time for somebody to see login + // instructions and go to a URL to do something.) + action = c.currentAction + if action.Accept || action.Reject { + if action.Reject { + metricTerminalReject.Add(1) + } else { + metricTerminalAccept.Add(1) + } + return action, nil + } + url := action.HoldAndDelegate + if url == "" { + metricTerminalMalformed.Add(1) + return nil, errors.New("reached Action that lacked Accept, Reject, and HoldAndDelegate") + } + metricHolds.Add(1) + url = c.expandDelegateURLLocked(url) + nextAction, err := c.fetchSSHAction(ctx, url) + if err != nil { + metricTerminalFetchError.Add(1) + return nil, fmt.Errorf("fetching SSHAction from %s: %w", url, err) + } + return nextAction, nil +} + func (c *conn) expandDelegateURLLocked(actionURL string) string { nm := c.srv.lb.NetMap() ci := c.info diff --git a/ssh/tailssh/tailssh_integration_test.go b/ssh/tailssh/tailssh_integration_test.go index 5c4f533b1..1799d3400 100644 --- a/ssh/tailssh/tailssh_integration_test.go +++ b/ssh/tailssh/tailssh_integration_test.go @@ -32,8 +32,8 @@ "github.com/bramvdbogaerde/go-scp" "github.com/google/go-cmp/cmp" "github.com/pkg/sftp" + gossh "github.com/tailscale/golang-x-crypto/ssh" "golang.org/x/crypto/ssh" - gossh "golang.org/x/crypto/ssh" "golang.org/x/crypto/ssh/agent" "tailscale.com/net/tsdial" "tailscale.com/tailcfg" diff --git a/ssh/tailssh/tailssh_test.go b/ssh/tailssh/tailssh_test.go index 207136659..9f3616d8c 100644 --- a/ssh/tailssh/tailssh_test.go +++ b/ssh/tailssh/tailssh_test.go @@ -31,7 +31,7 @@ "testing" "time" - gossh "golang.org/x/crypto/ssh" + gossh "github.com/tailscale/golang-x-crypto/ssh" "golang.org/x/net/http2" "golang.org/x/net/http2/h2c" "tailscale.com/ipn/ipnlocal" @@ -805,8 +805,7 @@ func TestSSHAuthFlow(t *testing.T) { state: &localState{ sshEnabled: true, }, - authErr: true, - wantBanners: []string{"tailscale: failed to evaluate SSH policy"}, + authErr: true, }, { name: "accept", diff --git a/tempfork/gliderlabs/ssh/agent.go b/tempfork/gliderlabs/ssh/agent.go index 99e84c1e5..86a5bce7f 100644 --- a/tempfork/gliderlabs/ssh/agent.go +++ b/tempfork/gliderlabs/ssh/agent.go @@ -7,7 +7,7 @@ "path" "sync" - gossh "golang.org/x/crypto/ssh" + gossh "github.com/tailscale/golang-x-crypto/ssh" ) const ( diff --git a/tempfork/gliderlabs/ssh/context.go b/tempfork/gliderlabs/ssh/context.go index 505a43dbf..d43de6f09 100644 --- a/tempfork/gliderlabs/ssh/context.go +++ b/tempfork/gliderlabs/ssh/context.go @@ -6,7 +6,7 @@ "net" "sync" - gossh "golang.org/x/crypto/ssh" + gossh "github.com/tailscale/golang-x-crypto/ssh" ) // contextKey is a value for use with context.WithValue. It's used as @@ -55,6 +55,8 @@ type contextKey struct { // ContextKeyPublicKey is a context key for use with Contexts in this package. // The associated value will be of type PublicKey. ContextKeyPublicKey = &contextKey{"public-key"} + + ContextKeySendAuthBanner = &contextKey{"send-auth-banner"} ) // Context is a package specific context interface. It exposes connection @@ -89,6 +91,8 @@ type Context interface { // SetValue allows you to easily write new values into the underlying context. SetValue(key, value interface{}) + + SendAuthBanner(banner string) error } type sshContext struct { @@ -117,6 +121,7 @@ func applyConnMetadata(ctx Context, conn gossh.ConnMetadata) { ctx.SetValue(ContextKeyUser, conn.User()) ctx.SetValue(ContextKeyLocalAddr, conn.LocalAddr()) ctx.SetValue(ContextKeyRemoteAddr, conn.RemoteAddr()) + ctx.SetValue(ContextKeySendAuthBanner, conn.SendAuthBanner) } func (ctx *sshContext) SetValue(key, value interface{}) { @@ -153,3 +158,7 @@ func (ctx *sshContext) LocalAddr() net.Addr { func (ctx *sshContext) Permissions() *Permissions { return ctx.Value(ContextKeyPermissions).(*Permissions) } + +func (ctx *sshContext) SendAuthBanner(msg string) error { + return ctx.Value(ContextKeySendAuthBanner).(func(string) error)(msg) +} diff --git a/tempfork/gliderlabs/ssh/options.go b/tempfork/gliderlabs/ssh/options.go index 29c8ef141..aa87a4f39 100644 --- a/tempfork/gliderlabs/ssh/options.go +++ b/tempfork/gliderlabs/ssh/options.go @@ -3,7 +3,7 @@ import ( "os" - gossh "golang.org/x/crypto/ssh" + gossh "github.com/tailscale/golang-x-crypto/ssh" ) // PasswordAuth returns a functional option that sets PasswordHandler on the server. diff --git a/tempfork/gliderlabs/ssh/options_test.go b/tempfork/gliderlabs/ssh/options_test.go index 47342b0f6..7cf6f376c 100644 --- a/tempfork/gliderlabs/ssh/options_test.go +++ b/tempfork/gliderlabs/ssh/options_test.go @@ -8,7 +8,7 @@ "sync/atomic" "testing" - gossh "golang.org/x/crypto/ssh" + gossh "github.com/tailscale/golang-x-crypto/ssh" ) func newTestSessionWithOptions(t *testing.T, srv *Server, cfg *gossh.ClientConfig, options ...Option) (*gossh.Session, *gossh.Client, func()) { diff --git a/tempfork/gliderlabs/ssh/server.go b/tempfork/gliderlabs/ssh/server.go index 473e5fbd6..1086a72ca 100644 --- a/tempfork/gliderlabs/ssh/server.go +++ b/tempfork/gliderlabs/ssh/server.go @@ -8,7 +8,7 @@ "sync" "time" - gossh "golang.org/x/crypto/ssh" + gossh "github.com/tailscale/golang-x-crypto/ssh" ) // ErrServerClosed is returned by the Server's Serve, ListenAndServe, diff --git a/tempfork/gliderlabs/ssh/session.go b/tempfork/gliderlabs/ssh/session.go index a7a9a3eeb..0a4a21e53 100644 --- a/tempfork/gliderlabs/ssh/session.go +++ b/tempfork/gliderlabs/ssh/session.go @@ -9,7 +9,7 @@ "sync" "github.com/anmitsu/go-shlex" - gossh "golang.org/x/crypto/ssh" + gossh "github.com/tailscale/golang-x-crypto/ssh" ) // Session provides access to information about an SSH session and methods diff --git a/tempfork/gliderlabs/ssh/session_test.go b/tempfork/gliderlabs/ssh/session_test.go index fe61a9d96..a60be5ec1 100644 --- a/tempfork/gliderlabs/ssh/session_test.go +++ b/tempfork/gliderlabs/ssh/session_test.go @@ -9,7 +9,7 @@ "net" "testing" - gossh "golang.org/x/crypto/ssh" + gossh "github.com/tailscale/golang-x-crypto/ssh" ) func (srv *Server) serveOnce(l net.Listener) error { diff --git a/tempfork/gliderlabs/ssh/ssh.go b/tempfork/gliderlabs/ssh/ssh.go index 54bd31ec2..644cb257d 100644 --- a/tempfork/gliderlabs/ssh/ssh.go +++ b/tempfork/gliderlabs/ssh/ssh.go @@ -4,7 +4,7 @@ "crypto/subtle" "net" - gossh "golang.org/x/crypto/ssh" + gossh "github.com/tailscale/golang-x-crypto/ssh" ) type Signal string @@ -105,7 +105,7 @@ type Pty struct { // requested by the client as part of the pty-req. These are outlined as // part of https://datatracker.ietf.org/doc/html/rfc4254#section-8. // - // The opcodes are defined as constants in golang.org/x/crypto/ssh (VINTR,VQUIT,etc.). + // The opcodes are defined as constants in github.com/tailscale/golang-x-crypto/ssh (VINTR,VQUIT,etc.). // Boolean opcodes have values 0 or 1. Modes gossh.TerminalModes } diff --git a/tempfork/gliderlabs/ssh/tcpip.go b/tempfork/gliderlabs/ssh/tcpip.go index 335fda657..056a0c734 100644 --- a/tempfork/gliderlabs/ssh/tcpip.go +++ b/tempfork/gliderlabs/ssh/tcpip.go @@ -7,7 +7,7 @@ "strconv" "sync" - gossh "golang.org/x/crypto/ssh" + gossh "github.com/tailscale/golang-x-crypto/ssh" ) const ( diff --git a/tempfork/gliderlabs/ssh/tcpip_test.go b/tempfork/gliderlabs/ssh/tcpip_test.go index b3ba60a9b..118b5d53a 100644 --- a/tempfork/gliderlabs/ssh/tcpip_test.go +++ b/tempfork/gliderlabs/ssh/tcpip_test.go @@ -10,7 +10,7 @@ "strings" "testing" - gossh "golang.org/x/crypto/ssh" + gossh "github.com/tailscale/golang-x-crypto/ssh" ) var sampleServerResponse = []byte("Hello world") diff --git a/tempfork/gliderlabs/ssh/util.go b/tempfork/gliderlabs/ssh/util.go index 3bee06dcd..e3b5716a3 100644 --- a/tempfork/gliderlabs/ssh/util.go +++ b/tempfork/gliderlabs/ssh/util.go @@ -5,7 +5,7 @@ "crypto/rsa" "encoding/binary" - "golang.org/x/crypto/ssh" + "github.com/tailscale/golang-x-crypto/ssh" ) func generateSigner() (ssh.Signer, error) { diff --git a/tempfork/gliderlabs/ssh/wrap.go b/tempfork/gliderlabs/ssh/wrap.go index d1f2b161e..17867d751 100644 --- a/tempfork/gliderlabs/ssh/wrap.go +++ b/tempfork/gliderlabs/ssh/wrap.go @@ -1,6 +1,6 @@ package ssh -import gossh "golang.org/x/crypto/ssh" +import gossh "github.com/tailscale/golang-x-crypto/ssh" // PublicKey is an abstraction of different types of public keys. type PublicKey interface {