code review 9853050: go.crypto/ssh: implement challenge/response auth (RFC 4...

go.crypto/ssh: implement keyboard-interactive auth (RFC 4256), both
on client and server-side.

[hanwen-google, 2013-05-31 22:22:17]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

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

[gobot, 2013-06-01 02:07:08]

R=dfc (assigned by dfc)

[dfc, 2013-06-06 12:04:42]

+cc agl,

I'm happy with the code, but I have no experience with this authentication
method, so I'll defer to agl on this aspect.

https://codereview.appspot.com/9853050/diff/7001/ssh/client_auth.go
File ssh/client_auth.go (right):

https://codereview.appspot.com/9853050/diff/7001/ssh/client_auth.go#newcode383
ssh/client_auth.go:383: questions []string, echos []bool) (answers []string, err
error)
please don't wrap, if the line is too long, drop the names on the return args.

https://codereview.appspot.com/9853050/diff/7001/ssh/client_auth.go#newcode429
ssh/client_auth.go:429: msg := decode(packet).(*userAuthFailureMsg)
please add a type assertion here, and return an error in the unlikely case.

https://codereview.appspot.com/9853050/diff/7001/ssh/client_auth.go#newcode437
ssh/client_auth.go:437: msg := new(userAuthInfoRequestMsg)
var msg userAuthInfoRequestMsg

https://codereview.appspot.com/9853050/diff/7001/ssh/client_auth.go#newcode438
ssh/client_auth.go:438: if err := unmarshal(msg, packet, packet[0]); err != nil
{
unmarshal(&msg, ...)

https://codereview.appspot.com/9853050/diff/7001/ssh/client_auth.go#newcode449
ssh/client_auth.go:449: return false, nil, errors.New("prompt format error")
I'm not sure what the tradition here is, I think people are leaning towards
adding ssh: prefixes to error messages.

https://codereview.appspot.com/9853050/diff/7001/ssh/client_auth_test.go
File ssh/client_auth_test.go (right):

https://codereview.appspot.com/9853050/diff/7001/ssh/client_auth_test.go#newc...
ssh/client_auth_test.go:118: questions []string, echos []bool) (answers
[]string, err error) {
same as previous file. As you are not using a naked return, I don't think the
named return args are adding much value here.

https://codereview.appspot.com/9853050/diff/7001/ssh/client_auth_test.go#newc...
ssh/client_auth_test.go:150: client.Challenge("user", "thank you for your
cooperation", nil, nil)
Is this a standard response message ?

https://codereview.appspot.com/9853050/diff/7001/ssh/common.go
File ssh/common.go (right):

https://codereview.appspot.com/9853050/diff/7001/ssh/common.go#newcode332
ssh/common.go:332: func appendString(buf []byte, s string) []byte {
OT: all these append helpers need a cleanup.

https://codereview.appspot.com/9853050/diff/7001/ssh/messages.go
File ssh/messages.go (right):

https://codereview.appspot.com/9853050/diff/7001/ssh/messages.go#newcode37
ssh/messages.go:37: // Method specific messages:
this is a good comment, s/://

Maybe add some more comments for the other groups

https://codereview.appspot.com/9853050/diff/7001/ssh/server.go
File ssh/server.go (right):

https://codereview.appspot.com/9853050/diff/7001/ssh/server.go#newcode46
ssh/server.go:46: // keyboard-interactive authentication is selected. The
please add a (RFC XXXX) which describes keyboard-interactive

https://codereview.appspot.com/9853050/diff/7001/ssh/server.go#newcode47
ssh/server.go:47: // passed-in client can be used to talk to the user.  The
s/passed-in client/function/

https://codereview.appspot.com/9853050/diff/7001/ssh/server.go#newcode548
ssh/server.go:548: return nil, errors.New("echos and questions must have equal
length")
please add an 'ssh: ' prefix

https://codereview.appspot.com/9853050/diff/7001/ssh/server.go#newcode591
ssh/server.go:591: return nil, errors.New("junk at end of message")
same

[hanwen-google, 2013-06-06 13:33:07]

thanks for the review! 

All addressed.

https://codereview.appspot.com/9853050/diff/7001/ssh/client_auth.go
File ssh/client_auth.go (right):

https://codereview.appspot.com/9853050/diff/7001/ssh/client_auth.go#newcode383
ssh/client_auth.go:383: questions []string, echos []bool) (answers []string, err
error)
On 2013/06/06 12:04:42, dfc wrote:
> please don't wrap, if the line is too long, drop the names on the return args.

Done.

https://codereview.appspot.com/9853050/diff/7001/ssh/client_auth.go#newcode429
ssh/client_auth.go:429: msg := decode(packet).(*userAuthFailureMsg)
On 2013/06/06 12:04:42, dfc wrote:
> please add a type assertion here, and return an error in the unlikely case.
>      

Done.

https://codereview.appspot.com/9853050/diff/7001/ssh/client_auth.go#newcode437
ssh/client_auth.go:437: msg := new(userAuthInfoRequestMsg)
On 2013/06/06 12:04:42, dfc wrote:
> var msg userAuthInfoRequestMsg

Done.

https://codereview.appspot.com/9853050/diff/7001/ssh/client_auth.go#newcode438
ssh/client_auth.go:438: if err := unmarshal(msg, packet, packet[0]); err != nil
{
On 2013/06/06 12:04:42, dfc wrote:
> unmarshal(&msg, ...)

Done.

https://codereview.appspot.com/9853050/diff/7001/ssh/client_auth.go#newcode449
ssh/client_auth.go:449: return false, nil, errors.New("prompt format error")
On 2013/06/06 12:04:42, dfc wrote:
> I'm not sure what the tradition here is, I think people are leaning towards
> adding ssh: prefixes to error messages.

Done.

https://codereview.appspot.com/9853050/diff/7001/ssh/client_auth_test.go
File ssh/client_auth_test.go (right):

https://codereview.appspot.com/9853050/diff/7001/ssh/client_auth_test.go#newc...
ssh/client_auth_test.go:118: questions []string, echos []bool) (answers
[]string, err error) {
On 2013/06/06 12:04:42, dfc wrote:
> same as previous file. As you are not using a naked return, I don't think the
> named return args are adding much value here.

Done.

https://codereview.appspot.com/9853050/diff/7001/ssh/client_auth_test.go#newc...
ssh/client_auth_test.go:150: client.Challenge("user", "thank you for your
cooperation", nil, nil)
On 2013/06/06 12:04:42, dfc wrote:
> Is this a standard response message ?

no, I was drinking a beer when writing this :)

anecdotally, it's empty for my current installation.

https://codereview.appspot.com/9853050/diff/7001/ssh/common.go
File ssh/common.go (right):

https://codereview.appspot.com/9853050/diff/7001/ssh/common.go#newcode332
ssh/common.go:332: func appendString(buf []byte, s string) []byte {
On 2013/06/06 12:04:42, dfc wrote:
> OT: all these append helpers need a cleanup.

sure . You want me to put a TODO somewhere? Open an issue?

(cleaned up in what way?)

https://codereview.appspot.com/9853050/diff/7001/ssh/messages.go
File ssh/messages.go (right):

https://codereview.appspot.com/9853050/diff/7001/ssh/messages.go#newcode37
ssh/messages.go:37: // Method specific messages:
On 2013/06/06 12:04:42, dfc wrote:
> this is a good comment, s/://
> 
> Maybe add some more comments for the other groups

Done.

https://codereview.appspot.com/9853050/diff/7001/ssh/server.go
File ssh/server.go (right):

https://codereview.appspot.com/9853050/diff/7001/ssh/server.go#newcode46
ssh/server.go:46: // keyboard-interactive authentication is selected. The
On 2013/06/06 12:04:42, dfc wrote:
> please add a (RFC XXXX) which describes keyboard-interactive

Done.

https://codereview.appspot.com/9853050/diff/7001/ssh/server.go#newcode47
ssh/server.go:47: // passed-in client can be used to talk to the user.  The
On 2013/06/06 12:04:42, dfc wrote:
> s/passed-in client/function/

it's an interface object, though, not a function.

https://codereview.appspot.com/9853050/diff/7001/ssh/server.go#newcode548
ssh/server.go:548: return nil, errors.New("echos and questions must have equal
length")
On 2013/06/06 12:04:42, dfc wrote:
> please add an 'ssh: ' prefix

Done.

https://codereview.appspot.com/9853050/diff/7001/ssh/server.go#newcode591
ssh/server.go:591: return nil, errors.New("junk at end of message")
On 2013/06/06 12:04:42, dfc wrote:
> same

Done.

[agl1, 2013-06-06 14:22:13]

LGTM

https://codereview.appspot.com/9853050/diff/23001/ssh/client_auth.go
File ssh/client_auth.go (right):

https://codereview.appspot.com/9853050/diff/23001/ssh/client_auth.go#newcode382
ssh/client_auth.go:382: // 3.3 details how the UI should behave for each both
CLI and
s/each//?

https://codereview.appspot.com/9853050/diff/23001/ssh/client_auth.go#newcode389
ssh/client_auth.go:389: func ClientAuthChallengeResponse(impl
ClientChallengeResponse) ClientAuth {
In OpenSSH, this is generally called "keyboard-interactive" I think. Perhaps
these should be so named? (i.e. ClientAuthKeyboardInteractive).

https://codereview.appspot.com/9853050/diff/23001/ssh/client_auth.go#newcode455
ssh/client_auth.go:455: echos = append(echos, (r[0] != 0))
I don't think the inner parentheses are needed here.

https://codereview.appspot.com/9853050/diff/23001/ssh/client_auth.go#newcode465
ssh/client_auth.go:465: return false, nil, errors.New("ssh: junk following
message")
errors.New seems wrong here. Pass err?

https://codereview.appspot.com/9853050/diff/23001/ssh/client_auth.go#newcode469
ssh/client_auth.go:469: return false, nil, errors.New("ssh: not enough answers")
"ssh: not enough answers from keyboard-interactive callback"

https://codereview.appspot.com/9853050/diff/23001/ssh/server.go
File ssh/server.go (right):

https://codereview.appspot.com/9853050/diff/23001/ssh/server.go#newcode49
ssh/server.go:49: // Challenge rounds.  To avoid information leaks, the client
only a single space after a period. (several cases in this comment.)

https://codereview.appspot.com/9853050/diff/23001/ssh/server.go#newcode433
ssh/server.go:433: ok := s.config.ChallengeResponseCallback(s, s.User,
&sshClientChallengeResponse{s})
merge this with following conditional.

https://codereview.appspot.com/9853050/diff/23001/ssh/server.go#newcode575
ssh/server.go:575: var n uint32
it seems that these two var lines can be eliminated by using := for parseUint32.

[hanwen-google, 2013-06-06 14:37:17]

https://codereview.appspot.com/9853050/diff/23001/ssh/client_auth.go
File ssh/client_auth.go (right):

https://codereview.appspot.com/9853050/diff/23001/ssh/client_auth.go#newcode382
ssh/client_auth.go:382: // 3.3 details how the UI should behave for each both
CLI and
On 2013/06/06 14:22:13, agl1 wrote:
> s/each//?

Done.

https://codereview.appspot.com/9853050/diff/23001/ssh/client_auth.go#newcode389
ssh/client_auth.go:389: func ClientAuthChallengeResponse(impl
ClientChallengeResponse) ClientAuth {
On 2013/06/06 14:22:13, agl1 wrote:
> In OpenSSH, this is generally called "keyboard-interactive" I think. Perhaps
> these should be so named? (i.e. ClientAuthKeyboardInteractive).

sshd_config actually calls it ChallengeResponseAuthentication, but the RFC
documents it as being officially named "keyboard-interactive", so changed it to
that.

https://codereview.appspot.com/9853050/diff/23001/ssh/client_auth.go#newcode455
ssh/client_auth.go:455: echos = append(echos, (r[0] != 0))
On 2013/06/06 14:22:13, agl1 wrote:
> I don't think the inner parentheses are needed here.

Done.

https://codereview.appspot.com/9853050/diff/23001/ssh/client_auth.go#newcode465
ssh/client_auth.go:465: return false, nil, errors.New("ssh: junk following
message")
On 2013/06/06 14:22:13, agl1 wrote:
> errors.New seems wrong here. Pass err?

Done.

https://codereview.appspot.com/9853050/diff/23001/ssh/client_auth.go#newcode469
ssh/client_auth.go:469: return false, nil, errors.New("ssh: not enough answers")
On 2013/06/06 14:22:13, agl1 wrote:
> "ssh: not enough answers from keyboard-interactive callback"

Done.

https://codereview.appspot.com/9853050/diff/23001/ssh/server.go
File ssh/server.go (right):

https://codereview.appspot.com/9853050/diff/23001/ssh/server.go#newcode49
ssh/server.go:49: // Challenge rounds.  To avoid information leaks, the client
On 2013/06/06 14:22:13, agl1 wrote:
> only a single space after a period. (several cases in this comment.)

Done.

https://codereview.appspot.com/9853050/diff/23001/ssh/server.go#newcode433
ssh/server.go:433: ok := s.config.ChallengeResponseCallback(s, s.User,
&sshClientChallengeResponse{s})
On 2013/06/06 14:22:13, agl1 wrote:
> merge this with following conditional.

Done.

https://codereview.appspot.com/9853050/diff/23001/ssh/server.go#newcode575
ssh/server.go:575: var n uint32
On 2013/06/06 14:22:13, agl1 wrote:
> it seems that these two var lines can be eliminated by using := for
parseUint32.

Done.

[agl1, 2013-06-06 14:44:20]

*** Submitted as
https://code.google.com/p/go/source/detail?r=c510c3e5067d&repo=crypto ***

go.crypto/ssh: implement keyboard-interactive auth (RFC 4256), both
on client and server-side.

R=dave, agl
CC=gobot, golang-dev
https://codereview.appspot.com/9853050

Committer: Adam Langley <agl@golang.org>

https://codereview.appspot.com/9853050/diff/28006/ssh/server.go
File ssh/server.go (right):

https://codereview.appspot.com/9853050/diff/28006/ssh/server.go#newcode433
ssh/server.go:433: if ok := s.config.KeyboardInteractiveCallback(s, s.User,
&sshClientKeyboardInteractive{s}); ok {
The 'ok' var isn't needed, but I'll fix that up when landing.

[hanwen-google, 2013-06-06 15:47:31]

*** Abandoned ***