code review 5373055: exp/ssh: add client side support for publickey auth

exp/ssh: add client side support for publickey auth

client.go/client_auth.go:
* add support for publickey key auth using the interface
  outlined by rsc in the previous auth CL

client_auth_test.go:
* password and publickey tests against server.go

common.go/server.go:
* move some helper methods from server.go into common.go
* generalise serializeRSASignature

[dave_cheney.net, 2011-11-11 05:04:33]

Hello rsc@golang.org, agl@golang.org, huin@google.com (cc: cw@f00f.org,
golang-dev@googlegroups.com, n13m3y3r@gmail.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[dave_cheney.net, 2011-11-11 05:07:46]

The authentication API is still a handful, it works, and is very flexible, but
arguably a handful for the simple case.

I have some ideas for improving the authentication API, but I would really like
to see the public key functionality committed first.

[agl1, 2011-11-11 15:26:00]

http://codereview.appspot.com/5373055/diff/4001/src/pkg/exp/ssh/client_auth.go
File src/pkg/exp/ssh/client_auth.go (right):

http://codereview.appspot.com/5373055/diff/4001/src/pkg/exp/ssh/client_auth.g...
src/pkg/exp/ssh/client_auth.go:160: var ErrNoKeys = errors.New("No more keys")
Needs comment and I think the naming scheme for errors would call this
"NoMoreKeysError". (Although the naming scheme might have changed so, if you
find examples of the Err* form, then go with that.)

http://codereview.appspot.com/5373055/diff/4001/src/pkg/exp/ssh/client_auth.g...
src/pkg/exp/ssh/client_auth.go:163: type ClientPublicKey interface {
|ClientPublicKey| suggests that it's a single key. ClientPublicKeyRing maybe?

http://codereview.appspot.com/5373055/diff/4001/src/pkg/exp/ssh/client_auth.g...
src/pkg/exp/ssh/client_auth.go:168: Key(i int) (alg string, pub []*big.Int, err
error)
Can |pub| not be an interface{} where the valid concrete types are rsa.PublicKey
and dsa.PublicKey? (http://golang.org/pkg/crypto/dsa/#PublicKey). We could get
rid of |alg| then.

Also, a nil interface could indicate "no more keys", leaving |err| for real
errors.

http://codereview.appspot.com/5373055/diff/4001/src/pkg/exp/ssh/client_auth.g...
src/pkg/exp/ssh/client_auth.go:171: Sign(i int, data []byte) (sig []byte, err
error)
If |Key| returns an rsa/dsa.*Private*Key, then I don't think we need Sign, we
can do the signing ourselves.

If you really want to keep Sign, you should pass in a io.Reader for random data.
It makes testing easier if uncontrolled entropy doesn't appear in the call
stack.

http://codereview.appspot.com/5373055/diff/4001/src/pkg/exp/ssh/client_auth.g...
src/pkg/exp/ssh/client_auth.go:184: Reply    bool
This argument isn't "Reply" it's "hasSignature" or something. If false, it means
that there's no signature and that the message is just a query whether the
public key would be valid, without the client having to decrypt it.

http://codereview.appspot.com/5373055/diff/4001/src/pkg/exp/ssh/client_auth.g...
src/pkg/exp/ssh/client_auth.go:194: alg, pub, err := p.Key(i)
It doesn't look like this tests the validity of the public key before trying it
(hasSignature = false). That's generally useful as it allows the client to
prompt the user only for the correct public key. Although this would require the
ClientPublicKeyRing interface to have two methods: GetPublicKey and
GetPrivateKey, with the latter only being called if the server indicates that
the public key is acceptable.

You can also send query messages for every public key without waiting and
collect the replies in one RTT.

[dave_cheney.net, 2011-11-12 01:33:35]

Hello rsc@golang.org, agl@golang.org, huin@google.com (cc: cw@f00f.org,
golang-dev@googlegroups.com, n13m3y3r@gmail.com),

Please take another look.

[dave_cheney.net, 2011-11-12 01:33:43]

Thank you for your comments agl, please take another look.

http://codereview.appspot.com/5373055/diff/4001/src/pkg/exp/ssh/client_auth.go
File src/pkg/exp/ssh/client_auth.go (right):

http://codereview.appspot.com/5373055/diff/4001/src/pkg/exp/ssh/client_auth.g...
src/pkg/exp/ssh/client_auth.go:160: var ErrNoKeys = errors.New("No more keys")
On 2011/11/11 15:26:00, agl1 wrote:
> Needs comment and I think the naming scheme for errors would call this
> "NoMoreKeysError". (Although the naming scheme might have changed so, if you
> find examples of the Err* form, then go with that.)

Done.

http://codereview.appspot.com/5373055/diff/4001/src/pkg/exp/ssh/client_auth.g...
src/pkg/exp/ssh/client_auth.go:163: type ClientPublicKey interface {
Good point, in practice there can only be a single ClientPublicKey active per
ClientConfig.[]Auth. I have renamed, and normalised the capitalisation to
Publickey.

On 2011/11/11 15:26:00, agl1 wrote:
> |ClientPublicKey| suggests that it's a single key. ClientPublicKeyRing maybe?

http://codereview.appspot.com/5373055/diff/4001/src/pkg/exp/ssh/client_auth.g...
src/pkg/exp/ssh/client_auth.go:171: Sign(i int, data []byte) (sig []byte, err
error)
In a previous CL I suggested supplying the password and/or the keys in the
ClientConfig but it was suggested that they actual signing shouldn't be done
inside the ssh package, but by a callback inside the clients code.

I'll take another swing at this retaining Sign for the moment, but if you feel
strongly about Key supplying the private key so ssh can do the signing I'm happy
to implement that.

On 2011/11/11 15:26:00, agl1 wrote:
> If |Key| returns an rsa/dsa.*Private*Key, then I don't think we need Sign, we
> can do the signing ourselves.
> 
> If you really want to keep Sign, you should pass in a io.Reader for random
data.
> It makes testing easier if uncontrolled entropy doesn't appear in the call
> stack.

On the subject of random and testing, would it work if ssh defined 

var rand = rand.Reader

and used that instead of the multitude of calls to rand.Reader currently spread
over the package. I have not tested this, but it may make testing easier as
well.

http://codereview.appspot.com/5373055/diff/4001/src/pkg/exp/ssh/client_auth.g...
src/pkg/exp/ssh/client_auth.go:184: Reply    bool
Will fix. I've started to adopt this method of defining a private struct so I
can offload as much of the packet construction to marshal.

On 2011/11/11 15:26:00, agl1 wrote:
> This argument isn't "Reply" it's "hasSignature" or something. If false, it
means
> that there's no signature and that the message is just a query whether the
> public key would be valid, without the client having to decrypt it.

http://codereview.appspot.com/5373055/diff/4001/src/pkg/exp/ssh/client_auth.g...
src/pkg/exp/ssh/client_auth.go:194: alg, pub, err := p.Key(i)
PTAL

On 2011/11/11 15:26:00, agl1 wrote:
> It doesn't look like this tests the validity of the public key before trying
it
> (hasSignature = false). That's generally useful as it allows the client to
> prompt the user only for the correct public key. Although this would require
the
> ClientPublicKeyRing interface to have two methods: GetPublicKey and
> GetPrivateKey, with the latter only being called if the server indicates that
> the public key is acceptable.
> 
> You can also send query messages for every public key without waiting and
> collect the replies in one RTT.

[agl1, 2011-11-13 16:50:55]

LGTM with the addition of an io.Reader for Sign.

I would make this change in landing, but it seems a little forceful. If you
agree with the rand io.Reader I can do so.


Cheers

http://codereview.appspot.com/5373055/diff/4001/src/pkg/exp/ssh/client_auth.go
File src/pkg/exp/ssh/client_auth.go (right):

http://codereview.appspot.com/5373055/diff/4001/src/pkg/exp/ssh/client_auth.g...
src/pkg/exp/ssh/client_auth.go:171: Sign(i int, data []byte) (sig []byte, err
error)
On 2011/11/11 15:26:00, agl1 wrote:
> If |Key| returns an rsa/dsa.*Private*Key, then I don't think we need Sign, we
> can do the signing ourselves.

Does Sign allow one to implement ssh agent support? That would be a great
argument for doing it this way.

In any case, I'm happy with this interface now, save for below:

> If you really want to keep Sign, you should pass in a io.Reader for random
data.
> It makes testing easier if uncontrolled entropy doesn't appear in the call
> stack.

I still believe that io.Reader should be passed in. We do this everywhere else.

http://codereview.appspot.com/5373055/diff/6007/src/pkg/exp/ssh/client_auth.go
File src/pkg/exp/ssh/client_auth.go (right):

http://codereview.appspot.com/5373055/diff/6007/src/pkg/exp/ssh/client_auth.g...
src/pkg/exp/ssh/client_auth.go:162: // Key returns the i'th rsa.Publickey or
dsa.Publickey, or nil if
It's PublicKey in rsa, dsa etc.

http://codereview.appspot.com/5373055/diff/6007/src/pkg/exp/ssh/client_auth.g...
src/pkg/exp/ssh/client_auth.go:219: packet, err := t.readPacket()
(Doesn't have to be sorted out in this CL.)

Doing each pubkey as a round trips means that n public keys are n RTTs of
latency. Instead we could send n public keys at once and read n replies, with
only a single RTT of latency.

[dave_cheney.net, 2011-11-13 19:23:41]

Agreed, and thanks

Sent from my iPhone

On 14/11/2011, at 3:50, agl@golang.org wrote:

> LGTM with the addition of an io.Reader for Sign.
> 
> I would make this change in landing, but it seems a little forceful. If
> you agree with the rand io.Reader I can do so.
> 
> 
> Cheers
> 
> 
> http://codereview.appspot.com/5373055/diff/4001/src/pkg/exp/ssh/client_auth.go
> File src/pkg/exp/ssh/client_auth.go (right):
> 
>
http://codereview.appspot.com/5373055/diff/4001/src/pkg/exp/ssh/client_auth.g...
> src/pkg/exp/ssh/client_auth.go:171: Sign(i int, data []byte) (sig
> []byte, err error)
> On 2011/11/11 15:26:00, agl1 wrote:
>> If |Key| returns an rsa/dsa.*Private*Key, then I don't think we need
> Sign, we
>> can do the signing ourselves.
> 
> Does Sign allow one to implement ssh agent support? That would be a
> great argument for doing it this way.
> 
> In any case, I'm happy with this interface now, save for below:
> 
>> If you really want to keep Sign, you should pass in a io.Reader for
> random data.
>> It makes testing easier if uncontrolled entropy doesn't appear in the
> call
>> stack.
> 
> I still believe that io.Reader should be passed in. We do this
> everywhere else.
> 
> http://codereview.appspot.com/5373055/diff/6007/src/pkg/exp/ssh/client_auth.go
> File src/pkg/exp/ssh/client_auth.go (right):
> 
>
http://codereview.appspot.com/5373055/diff/6007/src/pkg/exp/ssh/client_auth.g...
> src/pkg/exp/ssh/client_auth.go:162: // Key returns the i'th
> rsa.Publickey or dsa.Publickey, or nil if
> It's PublicKey in rsa, dsa etc.
> 
>
http://codereview.appspot.com/5373055/diff/6007/src/pkg/exp/ssh/client_auth.g...
> src/pkg/exp/ssh/client_auth.go:219: packet, err := t.readPacket()
> (Doesn't have to be sorted out in this CL.)
> 
> Doing each pubkey as a round trips means that n public keys are n RTTs
> of latency. Instead we could send n public keys at once and read n
> replies, with only a single RTT of latency.
> 
> http://codereview.appspot.com/5373055/

[dave_cheney.net, 2011-11-13 19:42:41]

Hello rsc@golang.org, agl@golang.org, huin@google.com (cc: cw@f00f.org,
golang-dev@googlegroups.com, n13m3y3r@gmail.com),

Please take another look.

[agl1, 2011-11-13 19:48:22]

*** Submitted as http://code.google.com/p/go/source/detail?r=c3ae7b7f6930 ***

exp/ssh: add client side support for publickey auth

client.go/client_auth.go:
* add support for publickey key auth using the interface
  outlined by rsc in the previous auth CL

client_auth_test.go:
* password and publickey tests against server.go

common.go/server.go:
* move some helper methods from server.go into common.go
* generalise serializeRSASignature

R=rsc, agl, huin
CC=cw, golang-dev, n13m3y3r
http://codereview.appspot.com/5373055

Committer: Adam Langley <agl@golang.org>

[dave_cheney.net, 2011-11-13 19:49:29]

I had some time before work so I added a rand io.Reader to the Sign method. This
has made the signature for the internal ClientAuth.auth method larger. I will
look at addressing that in a future CL.

http://codereview.appspot.com/5373055/diff/4001/src/pkg/exp/ssh/client_auth.go
File src/pkg/exp/ssh/client_auth.go (right):

http://codereview.appspot.com/5373055/diff/4001/src/pkg/exp/ssh/client_auth.g...
src/pkg/exp/ssh/client_auth.go:168: Key(i int) (alg string, pub []*big.Int, err
error)
On 2011/11/11 15:26:00, agl1 wrote:
> Can |pub| not be an interface{} where the valid concrete types are
rsa.PublicKey
> and dsa.PublicKey? (http://golang.org/pkg/crypto/dsa/#PublicKey). We could get
> rid of |alg| then.
> 
> Also, a nil interface could indicate "no more keys", leaving |err| for real
> errors.

Done.

http://codereview.appspot.com/5373055/diff/4001/src/pkg/exp/ssh/client_auth.g...
src/pkg/exp/ssh/client_auth.go:171: Sign(i int, data []byte) (sig []byte, err
error)
Yes, the idea is to delegate signing to something else, maybe even an external
process. 

On 2011/11/13 16:50:56, agl1 wrote:
> On 2011/11/11 15:26:00, agl1 wrote:
> > If |Key| returns an rsa/dsa.*Private*Key, then I don't think we need Sign,
we
> > can do the signing ourselves.
> 
> Does Sign allow one to implement ssh agent support? That would be a great
> argument for doing it this way.
> 
> In any case, I'm happy with this interface now, save for below:
> 
> > If you really want to keep Sign, you should pass in a io.Reader for random
> data.
> > It makes testing easier if uncontrolled entropy doesn't appear in the call
> > stack.
> 
> I still believe that io.Reader should be passed in. We do this everywhere
else.