ssh/tailssh: add back a fake public key handler to support buggy clients#14967
ssh/tailssh: add back a fake public key handler to support buggy clients#14967oxtoacart merged 1 commit intorelease-branch/1.80from
Conversation
41b2154 to
3892e78
Compare
awly
left a comment
There was a problem hiding this comment.
Do we also need to update nextAuthMethodCallback?
ssh/tailssh/tailssh.go
Outdated
| // We then accept any public key since we've already authenticated & authorized them. | ||
| func (c *conn) fakePublicKeyHandler(ctx ssh.Context, pubKey ssh.PublicKey) error { | ||
| if err := c.doPolicyAuth(ctx); err != nil { | ||
| // TODO(maisem/bradfitz): surface the error here. |
There was a problem hiding this comment.
I'm not sure what this comment means. It's logged and returned.
Does this mean sending a banner?
There was a problem hiding this comment.
Yeah, I'm not sure, I carried it over from the old code. I'll remove it.
ssh/tailssh/tailssh.go
Outdated
| // Note - tailssh_integration_test does not exercise this functionality because | ||
| // the client used for testing does not exhibit this buggy behavior. |
There was a problem hiding this comment.
You can drop "note" ... all comments are notes.
And then add a date, as this is a temporary comment. And a tracking issue.
// As of 2025-02-10, tailssh_integration_test doesn't .... See tailscale/tailscale#nnnn.
3892e78 to
378b701
Compare
No. This callback is only used if a previous auth method fails. We use it when someone supplies In the public key flow, the client is actually hitting the public key callback immediately. I will update the commit comment to clarify this. Note also that the original change that regressed this did not touch |
98e5676 to
55a8b8d
Compare
ssh/tailssh/tailssh.go
Outdated
| if err := c.isAuthorized(ctx); err != nil { | ||
| return err | ||
| } | ||
| c.logf("accepting SSH public key %s", bytes.TrimSpace(gossh.MarshalAuthorizedKey(pubKey))) |
There was a problem hiding this comment.
let's make this message a bit less misleading. It kinda suggests we care/know about that key
There was a problem hiding this comment.
and do we even care to log the MarshalAuthorizedKey value?
Maybe "confused client presented an unnecessary SSH public key; pacifying already-authenticated client by saying it's fine"
There was a problem hiding this comment.
I'll just take it out. We don't log results in the NoClientAuthCallback and I don't know that we need to here either.
Some clients don't support 'none' authentication and insist on supplying a public key. This change allows them to do so. It ignores the public key and uses Tailscale to authenticate. Updates #14922 Signed-off-by: Percy Wegmann <percy@tailscale.com>
55a8b8d to
f74f00f
Compare
|
Oh! You were doing that on the release branch. Please don't do that unless absolutely required. Try to develop on |
Some clients don't support 'none' authentication and insist on supplying
a public key. This change allows them to do so. It ignores the public key
and uses Tailscale to authenticate.
Updates #14922
I tested this by trying to tunnel pgadmin via a bastion host without and with the fix. Without the fix, it fails to even create the SSH tunnel.
With the fix, it gets further.