Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(238)

Issue 5373055: code review 5373055: exp/ssh: add client side support for publickey auth (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
13 years, 4 months ago by dave
Modified:
13 years, 4 months ago
Reviewers:
agl1, huin-google, rsc
CC:
cw, golang-dev, niemeyer
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : diff -r 68d902758434 https://go.googlecode.com/hg/ #

Patch Set 3 : diff -r 68d902758434 https://go.googlecode.com/hg/ #

Total comments: 14

Patch Set 4 : diff -r 7955d5c97b6a https://go.googlecode.com/hg/ #

Patch Set 5 : diff -r 7955d5c97b6a https://go.googlecode.com/hg/ #

Total comments: 2

Patch Set 6 : diff -r 01baaa1a6b5a https://go.googlecode.com/hg/ #

Patch Set 7 : diff -r 01baaa1a6b5a https://go.googlecode.com/hg/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+482 lines, -52 lines) Patch
M src/pkg/exp/ssh/client.go View 1 2 2 chunks +4 lines, -5 lines 0 comments Download
M src/pkg/exp/ssh/client_auth.go View 1 2 3 4 5 6 6 chunks +144 lines, -5 lines 0 comments Download
A src/pkg/exp/ssh/client_auth_test.go View 1 2 3 4 5 1 chunk +248 lines, -0 lines 0 comments Download
M src/pkg/exp/ssh/common.go View 1 2 3 4 2 chunks +85 lines, -0 lines 0 comments Download
M src/pkg/exp/ssh/server.go View 1 2 3 2 chunks +1 line, -42 lines 0 comments Download

Messages

Total messages: 10
dave_cheney.net
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 ...
13 years, 4 months ago (2011-11-11 05:04:33 UTC) #1
dave_cheney.net
The authentication API is still a handful, it works, and is very flexible, but arguably ...
13 years, 4 months ago (2011-11-11 05:07:46 UTC) #2
agl1
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.go#newcode160 src/pkg/exp/ssh/client_auth.go:160: var ErrNoKeys = errors.New("No more keys") Needs comment and ...
13 years, 4 months ago (2011-11-11 15:26:00 UTC) #3
dave_cheney.net
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.
13 years, 4 months ago (2011-11-12 01:33:35 UTC) #4
dave_cheney.net
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.go#newcode160 ...
13 years, 4 months ago (2011-11-12 01:33:43 UTC) #5
agl1
LGTM with the addition of an io.Reader for Sign. I would make this change in ...
13 years, 4 months ago (2011-11-13 16:50:55 UTC) #6
dave_cheney.net
Agreed, and thanks Sent from my iPhone On 14/11/2011, at 3:50, agl@golang.org wrote: > LGTM ...
13 years, 4 months ago (2011-11-13 19:23:41 UTC) #7
dave_cheney.net
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.
13 years, 4 months ago (2011-11-13 19:42:41 UTC) #8
agl1
*** 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: * ...
13 years, 4 months ago (2011-11-13 19:48:22 UTC) #9
dave_cheney.net
13 years, 4 months ago (2011-11-13 19:49:29 UTC) #10
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b