ssh/tailssh: accept passwords and public keys by oxtoacart · Pull Request #14970 · tailscale/tailscale

oxtoacart · 2025-02-10T18:44:41Z

Some clients don't request 'none' authentication. Instead, they immediately supply
a password or public key. This change allows them to do so, but ignores the supplied
credentials and authenticates using Tailscale instead.

Updates #14922

This applies #14967 to main. It's a substantially different change because main no longer uses the forked golang-x-crypto.

bradfitz · 2025-02-10T20:05:07Z

ssh/tailssh/tailssh.go

 		},
 		NoClientAuth: true, // required for the NoClientAuthCallback to run
 		NoClientAuthCallback: func(cm gossh.ConnMetadata) (*gossh.Permissions, error) {
+			c.logf("ZZZZ NoClientAuthCallback")


ssh/tailssh/tailssh.go

ssh/tailssh/testcontainers/Dockerfile

ssh/tailssh/tailssh_integration_test.go

ssh/tailssh/tailssh.go

bradfitz · 2025-02-11T00:01:48Z

ssh/tailssh/tailssh.go

 	}

+	if !c.clientAuthAttempted.CompareAndSwap(false, true) {
+		panic("clientAuth unexpectedly called twice")


can't they call NoClientAuthCallback and then later password or PublicKeyCallback?

when any of the callbacks returns a nil error, ssh.Server will stop the auth loop and shouldn't call any other callbacks: https://github.com/golang/crypto/blob/master/ssh/server.go#L798-L800

oh, and if they hit the +password hack, we swap the password callback for this one:

tailscale/ssh/tailssh/tailssh.go

Lines 387 to 395 in dd7ce90

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

},

},

}

}

So, this turned out to be a bit more complicated.

In cases where authentication succeeds, we'll only hit clientAuth() once. However, in cases where authentication fails, the client may try multiple authentication methods, for example none -> publickey -> password. Since we don't actually care about the content of the auth method, if the first method fails to authenticate, all subsequent ones will too.

What I'm doing with the new logic is tracking whether we've already attempted authentication. If so, we immediately return an error. Note that this is deliberately not a banner error, since we don't want to repeatedly signal the same failure to the client.

See here for the controlling loop. By default, the SSH server will try authentication up to 6 times. Each time, it reads an auth request from the client and then tries to do the authentication. If it succeeds, it breaks out of the loop, and we won't get any subsequent authentication attempts.

This behavior seems to be spec compliant. RFC4252 Section 5 says

While there is usually little point for clients to send requests that the server does not list as acceptable, sending such requests is not an error, and the server SHOULD simply reject requests that it does not recognize.

Also

the client MAY at any time continue with a new SSH_MSG_USERAUTH_REQUEST message, in which case the server MUST abandon the previous authentication attempt and continue with the new one.

ssh/tailssh/tailssh_integration_test.go

oxtoacart · 2025-02-11T14:47:50Z

ssh/tailssh/tailssh_integration_test.go

+from paramiko.ecdsakey import ECDSAKey
+client = pm.SSHClient()
+client.set_missing_host_key_policy(pm.AutoAddPolicy)
+client.connect('%s', port=%s, username='testuser', pkey=ECDSAKey.generate(), password='doesntmatter', allow_agent=False, look_for_keys=False)


This tests a client that does both public key and password auth.

oxtoacart · 2025-02-11T14:48:05Z

ssh/tailssh/tailssh_test.go

 	}
 	s := &server{
-		logf: logger.Discard,
+		logf: log.Printf,


Changed in order to see SSH logs during test.

log to the testing.T instead then. stderr logs are super spammy and intermingled and hard to read.

oxtoacart · 2025-02-11T14:48:36Z

ssh/tailssh/tailssh_test.go

+			if tc.authErr && !publicKeyUsed.Load() {
+				t.Error("public key should have been attempted")
+			}
+			if (tc.authErr || tc.usesPassword) && !passwordUsed.Load() {


Funnily enough, the old test never checked that the password callback was actually used. This now checks for that.

oxtoacart · 2025-02-11T14:48:47Z

ssh/tailssh/testcontainers/Dockerfile

+RUN if echo "$BASE" | grep "alpine:"; then apk add openssh python3 py3-pip; fi
+
+RUN echo "Install paramiko"
+RUN if echo "$BASE" | grep "ubuntu:"; then pip3 install paramiko==3.5.1; fi


Paramiko version is now pinned.

ssh/tailssh/tailssh.go

awly · 2025-02-11T16:45:09Z

tempfork/sshtest/ssh/client_auth.go

+		if config.SkipNoneAuth && !skippedNoneAuth {
+			skippedNoneAuth = true
+			continue
+		}


IIUC, if you simply continue here, the loop will enter again and use the same initial auth value of none.
I think you need it to reach the findNext block below to update the auth method.

Yes thanks, I've changed this quite a bit. I'm now initializing auth based on whether SkipNoneAuth is configured.

awly · 2025-02-11T16:46:12Z

ssh/tailssh/tailssh_test.go

+						testssh.PasswordCallback(func() (secret string, err error) {
+							// This 2nd password callback simulates someone entering a password a
+							// 2nd time.
+							passwordUsed.Store(true)


optional: you can make this into atomic.Uint32 to make sure both passwords were attempted

It turns out that the Go SSH client only attempts the first method of each type anyway, so I took this out.

bradfitz · 2025-02-11T19:57:43Z

ssh/tailssh/tailssh.go


 			return perms, nil
 		},
+		PasswordCallback: func(cm gossh.ConnMetadata, pword []byte) (*gossh.Permissions, error) {


But by even having this defined, doesn't that make us advertise it as a valid thing to try next if "none" fails? So then an SSH client can ask for or pop open a password prompt, thinking it might work since we advertised "password" as a next authentication types after the "none" failed?

Yes, having this here means that we respond to the none auth method with both publickey and password as acceptable methods.

In the interest of minimizing changes, I think we should take this out for now to focus just on achieving parity with 1.80, then we can revisit this question later.

Ugh. I miss our NextAuthMethodCallback. :(

Oh! We can use PartialSuccessError I think!

That seems to have done the trick!

bradfitz · 2025-02-11T21:30:51Z

ssh/tailssh/tailssh.go

 	sshDisableForwarding = envknob.RegisterBool("TS_SSH_DISABLE_FORWARDING")
 	sshDisablePTY        = envknob.RegisterBool("TS_SSH_DISABLE_PTY")
+
+	// terminalError is an empty PartialSuccessError, which results in the ssh


... is an empty PartialSuccessError (with no 'Next' authentication methods that may proceed), which results ...

and "SSH server" (since it's in prose)

... "immediately disconnecting the client without sending the 'Partial' bit on the wire"

bradfitz · 2025-02-11T21:31:32Z

ssh/tailssh/tailssh.go

-// 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 {
+// bannerError writes the given message to an auth banner  and then returns an


accidental double space

bradfitz · 2025-02-11T21:33:11Z

ssh/tailssh/tailssh.go

+// policy. It writes the message to an auth banner and then returns an empty
+// gossh.PartialSuccessError in order to stop processing authentication
+// attempts and immediately disconnect the client.
+func (c *conn) errDenied(message string) *gossh.PartialSuccessError {


kinda weird that this is "errFoo" but below you have methods that are "fooError"

Also weird that you're returning a concrete error type .... this is the classic mistake that's documented at https://go.dev/doc/faq#nil_error but I guess you're always returning a non-nil error at least, so probably okay.

Yeah, I had made a deliberate choice to return the concrete error type to ensure that no one accidentally changes this to return a different error type, because that would break the invariant that this error should result in authentication terminating. But since I added a check in the defer() in clientAuth(), I don't need this anymore.

bradfitz · 2025-02-11T21:33:49Z

ssh/tailssh/tailssh.go

+// being tried.
+func (c *conn) clientAuth(cm gossh.ConnMetadata) (perms *gossh.Permissions, finalErr error) {
+	defer func() {
+		if finalErr != nil {


delete and unindent this?

bradfitz · 2025-02-11T21:34:01Z

ssh/tailssh/tailssh.go

+					pse.Next.PublicKeyCallback != nil {
+					panic("clientAuth attempted to return a non-empty PartialSuccessError")
+				}
+			} else {


} else if finalErr != nil {

bradfitz · 2025-02-11T21:35:18Z

ssh/tailssh/tailssh.go

+// terminalError sends an empty gossh.PartialSuccessError to tell gossh.Server
+// to stop processing authentication attempts and instead disconnect
+// immediately.
+func (c *conn) terminalError(err error) *gossh.PartialSuccessError {


when does one use terminalError vs errDenied?

As the doc comment for errDenied states, errDenied is returned by auth callbacks when a connection is denied by the policy. I'll rename terminalError to errUnexpected and document that it's used when encountering unexpected conditions (like being unable to send an auth banner at all).

Comment on errUnexpected updated.

bradfitz · 2025-02-11T21:37:32Z

ssh/tailssh/tailssh_test.go

+					SkipNoneAuth:    skipNoneAuth,
+					Auth: []testssh.AuthMethod{
+						testssh.PublicKeysCallback(func() (signers []testssh.Signer, err error) {


you're setting SkipNoneAuth on half the tests, but how do you control whether it starts with a password vs starts with a public key if you're setting both?

I wasn't worrying about the order because when authentication fails, I expect both callbacks to be tried. Being explicitly would be better though.

Done, the tests now have three passes:

with none auth

without none auth starting with publickey

without none auth starting with password

oxtoacart · 2025-02-12T13:20:23Z

ssh/tailssh/tailssh.go

 		// 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, errors.New("server is shutting down")


Drive-by: there was no point to returning errDenied here since we don't even have an SSH connection at this point.

bradfitz · 2025-02-12T19:05:27Z

ssh/tailssh/tailssh.go

+	// terminalError is an empty gossh.PartialSuccessError (with no 'Next'
+	// authentication methods that may proceed), which results in the SSH
+	// server immediately disconnecting the client.
+	terminalError = &gossh.PartialSuccessError{}


error variables are named errFoo (or ErrFoo) in Go, and types are named fooError (or FooError)

So this would be errTerminal or errNoMoreAuthTypes if you want to be more explicit.

bradfitz · 2025-02-12T19:07:03Z

ssh/tailssh/tailssh.go

+// If access is denied, it returns an error. This must always be an empty
+// gossh.PartialSuccessError to prevent further authentication methods from
+// being tried.
+func (c *conn) clientAuth(cm gossh.ConnMetadata) (perms *gossh.Permissions, finalErr error) {


instead of "final", I think we normally use "retErr" (short for returned error). grep shows that retErr is pretty common

bradfitz · 2025-02-12T19:08:10Z

ssh/tailssh/tailssh_integration_test.go

@@ -410,6 +410,48 @@ func TestSSHAgentForwarding(t *testing.T) {
 	}
 }



btw, remove the ancient // +build integrationtest from the top of this file. It's the only such old one in the tree.

bradfitz · 2025-02-12T19:12:36Z

ssh/tailssh/tailssh_test.go

+				// When skipping `none` auth, the first callback should always
+				// fire, and the 2nd callback should fire only if
+				// authentication failed.
+				expectPublicKey := false


Go generally uses word "want" instead of "expect" in tests

Some clients don't request 'none' authentication. Instead, they immediately supply a password or public key. This change allows them to do so, but ignores the supplied credentials and authenticates using Tailscale instead. Updates #14922 Signed-off-by: Percy Wegmann <percy@tailscale.com>

oxtoacart force-pushed the percy/issue14922-main branch 2 times, most recently from c4fa9d9 to d54f95c Compare February 10, 2025 19:14

bradfitz reviewed Feb 10, 2025

View reviewed changes

ssh/tailssh/tailssh.go Show resolved Hide resolved

oxtoacart force-pushed the percy/issue14922-main branch from d54f95c to 7b22f56 Compare February 10, 2025 20:59

oxtoacart changed the title ~~ssh/tailssh: accept passwords and public keys~~ ssh/tailssh: accept public keys Feb 10, 2025

oxtoacart force-pushed the percy/issue14922-main branch 4 times, most recently from 29372a9 to dd7ce90 Compare February 10, 2025 23:51

oxtoacart requested a review from awly February 10, 2025 23:52

bradfitz reviewed Feb 10, 2025

View reviewed changes

ssh/tailssh/testcontainers/Dockerfile Outdated Show resolved Hide resolved

awly reviewed Feb 11, 2025

View reviewed changes

ssh/tailssh/tailssh_integration_test.go Outdated Show resolved Hide resolved

ssh/tailssh/tailssh.go Show resolved Hide resolved

bradfitz reviewed Feb 11, 2025

View reviewed changes

ssh/tailssh/tailssh.go Outdated Show resolved Hide resolved

bradfitz reviewed Feb 11, 2025

View reviewed changes

oxtoacart force-pushed the percy/issue14922-main branch from dd7ce90 to 3288ffb Compare February 11, 2025 14:46

oxtoacart changed the title ~~ssh/tailssh: accept public keys~~ ssh/tailssh: accept passwords and public keys Feb 11, 2025

oxtoacart commented Feb 11, 2025

View reviewed changes

ssh/tailssh/tailssh_integration_test.go Show resolved Hide resolved

oxtoacart commented Feb 11, 2025

View reviewed changes

oxtoacart force-pushed the percy/issue14922-main branch 7 times, most recently from 97b7752 to c59da3d Compare February 11, 2025 16:18

awly approved these changes Feb 11, 2025

View reviewed changes

oxtoacart force-pushed the percy/issue14922-main branch 4 times, most recently from 6e589ae to a5f5260 Compare February 11, 2025 17:03

bradfitz reviewed Feb 11, 2025

View reviewed changes

oxtoacart force-pushed the percy/issue14922-main branch 2 times, most recently from 11a880d to 8060d34 Compare February 11, 2025 21:21

bradfitz reviewed Feb 11, 2025

View reviewed changes

oxtoacart force-pushed the percy/issue14922-main branch 2 times, most recently from 88dfdf9 to 515cef4 Compare February 12, 2025 13:19

oxtoacart commented Feb 12, 2025

View reviewed changes

bradfitz approved these changes Feb 12, 2025

View reviewed changes

oxtoacart force-pushed the percy/issue14922-main branch from 515cef4 to 1935ab4 Compare February 12, 2025 20:51

bradfitz approved these changes Feb 12, 2025

View reviewed changes

oxtoacart force-pushed the percy/issue14922-main branch from 1935ab4 to 1a09996 Compare February 12, 2025 22:45

oxtoacart merged commit db23110 into main Feb 13, 2025

oxtoacart deleted the percy/issue14922-main branch February 13, 2025 17:29

	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
	},
	},
	}
	}

		@@ -410,6 +410,48 @@ func TestSSHAgentForwarding(t *testing.T) {
		}
		}

Conversation

oxtoacart commented Feb 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oxtoacart Feb 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oxtoacart Feb 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

oxtoacart commented Feb 10, 2025 •

edited

Loading

oxtoacart Feb 11, 2025 •

edited

Loading

oxtoacart Feb 11, 2025 •

edited

Loading