code review 5577070: go.crypto/ssh: add user name and netowrk address to ser...

go.crypto/ssh: add user name and netowrk address to server connection.

[dave_cheney.net, 2012-01-29 10:29:41]

+agl1

This looks ok, leaving for AGL.

http://codereview.appspot.com/5577070/diff/3/ssh/server.go
File ssh/server.go (right):

http://codereview.appspot.com/5577070/diff/3/ssh/server.go#newcode158
ssh/server.go:158: ip net.IP
I don't think this needs to be a field, it can be computed when needed.

http://codereview.appspot.com/5577070/diff/3/ssh/server.go#newcode168
ssh/server.go:168: ip:        c.RemoteAddr().(*net.TCPAddr).IP,
I don't think you can assume that c is a *net.TCPConn, so you can't assume that
RemoteAddr() will return a *net.TCPAddr.

I know I suggested passing down the net.IP to the authentication methods, but in
light of seeing this problem, maybe passing the net.Addr() down to the authn
functions who can take the load of doing a type switch on the net.Addr.

They should also consider the case where net.Addr is not valid, because it is
not known, probably by proxying.

[kardia, 2012-01-29 17:31:25]

Hello golang-dev@googlegroups.com, dave@cheney.net (cc: agl@golang.org,
golang-dev@googlegroups.com),

I'd like you to review this change to
http://code.google.com/p/go.crypto

[dave_cheney.net, 2012-01-30 12:07:09]

I'm having a few problems getting my go.crypto subrepo setup to review
this patch. I'll attempt to do so again tomorrow.

Have you considered how the change in the authn callbacks will affect
the ssh package's tests?

On Mon, Jan 30, 2012 at 4:31 AM,  <kardianos@gmail.com> wrote:
> Reviewers: golang-dev_googlegroups.com, dfc,
>
> Message:
> Hello golang-dev@googlegroups.com, dave@cheney.net (cc: agl@golang.org,
> golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> http://code.google.com/p/go.crypto
>
>
> Description:
> go.crypto/ssh: add user name and netowrk address to server connection.
>
> Please review this at http://codereview.appspot.com/5577070/
>
> Affected files:
>  M ssh/server.go
>
>
> Index: ssh/server.go
> ===================================================================
> --- a/ssh/server.go
> +++ b/ssh/server.go
> @@ -34,12 +34,12 @@
>        // PasswordCallback, if non-nil, is called when a user attempts to
>        // authenticate using a password. It may be called concurrently from
>        // several goroutines.
> -       PasswordCallback func(user, password string) bool
> +       PasswordCallback func(ip net.Addr, user, password string) bool
>
>        // PublicKeyCallback, if non-nil, is called when a client attempts
> public
>        // key authentication. It must return true iff the given public key
> is
>        // valid for the given user.
> -       PublicKeyCallback func(user, algo string, pubkey []byte) bool
> +       PublicKeyCallback func(ip net.Addr, user, algo string, pubkey
> []byte) bool
>
>        // Cryptographic-related configuration.
>        Crypto CryptoConfig
> @@ -150,6 +150,9 @@
>        // before attempting to authenticate with it, we end up with
> duplicate
>        // queries for public key validity.
>        cachedPubKeys []cachedPubKey
> +
> +       // user holds the successfully authenticated user name.
> +       user string
>  }
>
>  // Server returns a new SSH server connection
> @@ -369,7 +372,7 @@
>                }
>        }
>
> -       result := s.config.PublicKeyCallback(user, algo, pubKey)
> +       result := s.config.PublicKeyCallback(s.transport.RemoteAddr(), user,
> algo, pubKey)
>        if len(s.cachedPubKeys) < maxCachedPubKeys {
>                c := cachedPubKey{
>                        user:   user,
> @@ -421,7 +424,8 @@
>                                return ParseError{msgUserAuthRequest}
>                        }
>
> -                       if s.config.PasswordCallback(userAuthReq.User,
> string(password)) {
> +                       if
> s.config.PasswordCallback(s.transport.RemoteAddr(), userAuthReq.User,
> string(password)) {
> +                               s.user = userAuthReq.User
>                                break userAuthLoop
>                        }
>                case "publickey":
> @@ -490,6 +494,7 @@
>                                        return errors.New("ssh:
> isAcceptableAlgo incorrect")
>                                }
>                                if s.testPubKey(userAuthReq.User, algo,
> pubKey) {
> +                                       s.user = userAuthReq.User
>                                        break userAuthLoop
>                                }
>                        }
> @@ -634,6 +639,17 @@
>        panic("unreachable")
>  }
>
> +// User returns the authenticated user.
> +// It is empty if no authentication is used.
> +func (s *ServerConn) User() string {
> +       return s.user
> +}
> +
> +// RemoteAddr returns the connected client address.
> +func (s *ServerConn) RemoteAddr() net.Addr {
> +       return s.transport.RemoteAddr()
> +}
> +
>  // A Listener implements a network listener (net.Listener) for SSH
> connections.
>  type Listener struct {
>        listener net.Listener
>
>

[kardia, 2012-01-30 13:20:19]

Hello golang-dev@googlegroups.com, dave@cheney.net (cc: agl@golang.org,
golang-dev@googlegroups.com),

Please take another look.

[agl1, 2012-01-30 15:05:37]

User() seems good, but why not just ServerConn.User? (i.e. an exported string?)

Since transport is an anonymous field, I think RemoteAddr will already be
exposed in ServerConn.

At that point, rather than passing a net.Addr to the callbacks, why not pass the
*ServerConn? In this patch or in the future we can also add a UserData
interface{} to ServerConn so that folks can hang their own data off it as
needed. (Better names than UserData happily accepted.)

[kardia, 2012-01-30 19:19:00]

Hello golang-dev@googlegroups.com, dave@cheney.net, agl@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[agl1, 2012-01-30 19:24:55]

LGTM. Will land unless there are objections in the next day.

[kardia, 2012-01-30 20:01:17]

Hello golang-dev@googlegroups.com, dave@cheney.net, agl@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[dave_cheney.net, 2012-01-30 20:03:16]

LGTM. Thanks.

[agl1, 2012-01-30 20:30:27]

*** Submitted as 943eeb94bdc1 ***

go.crypto/ssh: add User to ServerConn and pass *ServerConn to callbacks.

R=golang-dev, dave, agl
CC=golang-dev
http://codereview.appspot.com/5577070

Committer: Adam Langley <agl@golang.org>