Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

x/crypto/ssh: improve error message on missing AuthMethod #33210

Closed
philippgille opened this issue Jul 21, 2019 · 8 comments
Closed

x/crypto/ssh: improve error message on missing AuthMethod #33210

philippgille opened this issue Jul 21, 2019 · 8 comments
Labels
FrozenDueToAge
Milestone
Unreleased

Comments

@philippgille
Copy link

philippgille commented Jul 21, 2019 •
edited

What version of Go are you using (go version)?

$ go version
go version go1.12.7 windows/amd64

Does this issue reproduce with the latest release?

Above mentioned version is currently the latest release.

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
set GOARCH=amd64
set GOBIN=
set GOCACHE=
set GOEXE=.exe
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOOS=windows
set GOPATH=
set GOPROXY=
set GORACE=
set GOROOT=
set GOTMPDIR=
set GOTOOLDIR=
set GCCGO=gccgo
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -mthreads -fmessage-length=0 -fdebug-prefix-map==/tmp/go-build -gno-record-gcc-switches

What did you do?

  1. Create an ssh.ClientConfig with nil as ssh.AuthMethod
  2. ssh.Dial() to an SSH server that requires either public key or keyboard interactive authentication

See https://play.golang.org/p/GYyQK6uhS3Q

What did you expect to see?

I expected ssh.Dial() to return an error with a useful message, like "The server sent this list of allowed auth methods: public key, keyboard interactive. But you didn't pass any".

Passing nil here isn't a problem by itself. It works fine when connecting to an SSH server that uses none as auth method. See https://play.golang.org/p/Gpro3QgsyFJ.

What did you see instead?

panic: runtime error: invalid memory address or nil pointer dereference
[signal 0xc0000005 code=0x0 addr=0x20 pc=0x5239a7]

goroutine 1 [running]:
golang.org/x/crypto/ssh.(*connection).clientAuthenticate(0xc0000ba100, 0xc000060750, 0x0, 0xa)
        X:/path/to/go/src/golang.org/x/crypto/ssh/client_auth.go:63 +0x437
golang.org/x/crypto/ssh.(*connection).clientHandshake(0xc0000ba100, 0x597f0c, 0xd, 0xc000060750, 0x0, 0x0)
        X:/path/to/go/src/golang.org/x/crypto/ssh/client.go:113 +0x2c2
golang.org/x/crypto/ssh.NewClientConn(0x5c70c0, 0xc000094020, 0x597f0c, 0xd, 0xc000091ec0, 0x5c70c0, 0xc000094020, 0x0, 0x0, 0xc000020501, ...)
        X:/path/to/go/src/golang.org/x/crypto/ssh/client.go:83 +0x105
golang.org/x/crypto/ssh.Dial(0x596630, 0x3, 0x597f0c, 0xd, 0xc000091ec0, 0xc000091ef8, 0x13b334a7, 0xefaa1b04ae02bf13)
        X:/path/to/go/src/golang.org/x/crypto/ssh/client.go:177 +0xba
main.main()
        X:/path/to/go/src/temp/tunnel/main.go:17 +0x121
exit status 2

Workaround

Workaround for others who come across this issue: If you don't know which auth methods are supported by the SSH server you're trying to connect to, use the verbose flag: ssh -v your.server.com. The output then contains for example these lines:

[...]
debug1: SSH2_MSG_SERVICE_ACCEPT received
debug1: Authentications that can continue: publickey,keyboard-interactive
debug1: Next authentication method: publickey
[...]

I'm sure there's also a way to do this programmatically in Go, but I didn't check yet.
@gopherbot gopherbot added this to the Unreleased milestone Jul 21, 2019
@FiloSottile FiloSottile changed the title x/crypto: Improve error message on missing ssh.AuthMethod x/crypto/ssh: improve error message on missing AuthMethod Jul 22, 2019
@agnivade
Copy link
Contributor

@hanwen

@hanwen
Copy link
Contributor

hanwen commented Jul 22, 2019

I don't understand the request.

You're specifying nil as input and you get a nil-pointer error; this seems like it's working as indended?

@philippgille
Copy link
Author

If using nil would always lead to an error then maybe, although I'd think even in that case a returned error with a useful message would be much nicer. Shouldn't a panic be avoided in libraries if possible?

But in this case the thing is even that it's not always leading to a panic. Only when connecting to an SSH server that doesn't have none explicitly set as auth method. This might make sense from the library's point of view ("the auth method is only checked if the server requires one, so the panic only happens in that case"), but as a library user I might want to do something like this: "A user uses my CLI tool to connect to his SSH server but didn't pass his credentials as parameter, so I can't set an auth method which works just fine if his server requires no auth. I'm sure there will be a proper error returned by the library in case the server turns out to actually require auth, so I can show an error message to the user." - and then instead of an error get a panic that doesn't give you much context around what happened.

As I mentioned in my initial comment: Why not return an error with a message like "The server sent this list of allowed auth methods: public key, keyboard interactive. But you didn't pass any"?

@hanwen
Copy link
Contributor

hanwen commented Jul 23, 2019

I think it is OK for panic to occur if you are using the library outside of spec. See e.g. https://play.golang.org/p/h93s6C-N5gy

@philippgille
Copy link
Author

But there's the difference I mentioned: zip.NewReader(nil, 1024) never works and no one would expect it to work, while an ssh.ClientConfig with nil as auth method element does work when the server requires no authentication.

But this lead me to try out something I didn't try yet: Instead of using nil as an element of the auth method slice, I didn't set any at all. I didn't try this yet because in my code a single ssh.AuthMethod is passed as parameter so I always created a ssh.ClientConfig with the auth method slice and only set the passed parameter as value.
Without any auth method slice it looks like this: https://play.golang.org/p/0PKqIwYTeN7
And instead of a panic an error was actually returned with useful info: ssh: handshake failed: ssh: unable to authenticate, attempted methods [none], no supported methods remain

This "discovery" makes me think this might be fine.

@hanwen
Copy link
Contributor

hanwen commented Jul 24, 2019

if the remote end doesn't need authentication, there is no need to inspect the auth methods at all.

@hanwen
Copy link
Contributor

hanwen commented Jul 24, 2019

shall we close this bug then?

@philippgille
Copy link
Author

The issue can be closed because with ssh.ClientConfig.Auth not being set at all, the library behaves exactly as I expected.

But I still have the feeling that you don't understand my point and even if an unset ssh.ClientConfig.Auth would lead to a panic you would say that it works as intended. The issue was (from the beginning) about the usability, that's why the title is "improve error message", and not "the library has a bug".

@golang golang locked and limited conversation to collaborators Jul 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
4 participants
@hanwen @philippgille @agnivade @gopherbot