code review 13528044: go.crypto/ssh: Begin adding server side support for mor...

go.crypto/ssh: Begin adding server side support for more than RSA for client key auth

[jmpittman, 2013-09-04 17:18:31]

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

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

[agl1, 2013-09-04 17:47:54]

LGTM with nits.

https://codereview.appspot.com/13528044/diff/6001/ssh/common.go
File ssh/common.go (right):

https://codereview.appspot.com/13528044/diff/6001/ssh/common.go#newcode214
ssh/common.go:214: func getHashFunc(algo string) (hf crypto.Hash, ok bool) {
I wouldn't name the return values here.

https://codereview.appspot.com/13528044/diff/6001/ssh/common.go#newcode217
ssh/common.go:217: hf = crypto.SHA1
for each of these, I'd just say "return crypto.SHA1, true" (for example)

https://codereview.appspot.com/13528044/diff/6001/ssh/server.go
File ssh/server.go (right):

https://codereview.appspot.com/13528044/diff/6001/ssh/server.go#newcode481
ssh/server.go:481: return algo == KeyAlgoRSA || algo == KeyAlgoDSA ||
this is a switch.

[jmpittman, 2013-09-04 18:15:01]

https://codereview.appspot.com/13528044/diff/6001/ssh/common.go
File ssh/common.go (right):

https://codereview.appspot.com/13528044/diff/6001/ssh/common.go#newcode214
ssh/common.go:214: func getHashFunc(algo string) (hf crypto.Hash, ok bool) {
On 2013/09/04 17:47:54, agl1 wrote:
> I wouldn't name the return values here.

Done.

https://codereview.appspot.com/13528044/diff/6001/ssh/common.go#newcode217
ssh/common.go:217: hf = crypto.SHA1
On 2013/09/04 17:47:54, agl1 wrote:
> for each of these, I'd just say "return crypto.SHA1, true" (for example)

Done.

https://codereview.appspot.com/13528044/diff/6001/ssh/server.go
File ssh/server.go (right):

https://codereview.appspot.com/13528044/diff/6001/ssh/server.go#newcode481
ssh/server.go:481: return algo == KeyAlgoRSA || algo == KeyAlgoDSA ||
On 2013/09/04 17:47:54, agl1 wrote:
> this is a switch.

Done.

https://codereview.appspot.com/13528044/diff/6001/ssh/server.go#newcode609
ssh/server.go:609: if !isAcceptableAlgo(algo) || !isAcceptableAlgo(sig.Format)
|| !strings.HasPrefix(algo, sig.Format) {
I am wondering if the HasPrefix check here makes sense or if a new function
should be created to return the private key algorithm name that corresponds to a
public key algorithm name.  For most key types, the name will be the same.  For
certificates, it will differ.  Off the top of my head, I cannot think of any
other situations like this.  Thoughts?

[agl1, 2013-09-05 15:21:50]

https://codereview.appspot.com/13528044/diff/6001/ssh/server.go
File ssh/server.go (right):

https://codereview.appspot.com/13528044/diff/6001/ssh/server.go#newcode609
ssh/server.go:609: if !isAcceptableAlgo(algo) || !isAcceptableAlgo(sig.Format)
|| !strings.HasPrefix(algo, sig.Format) {
On 2013/09/04 18:15:01, jmpittman wrote:
> I am wondering if the HasPrefix check here makes sense or if a new function
> should be created to return the private key algorithm name that corresponds to
a
> public key algorithm name.  For most key types, the name will be the same. 
For
> certificates, it will differ.  Off the top of my head, I cannot think of any
> other situations like this.  Thoughts?

I'm afraid that I've lost what is going on here. (Which probably means that the
comment could be expanded.)

The code is dealing with the cert formats from [1]? And it's checking that an
"ssh-rsa" signature type is a prefix of "ssh-rsa-cert-v01@openssh.org"? A prefix
seems ok, although an explicit function (with comment!) would be better. 

[1]
http://www.openbsd.org/cgi-bin/cvsweb/src/usr.bin/ssh/PROTOCOL.certkeys?rev=1...

[jmpittman, 2013-09-05 16:00:08]

On 2013/09/05 15:21:50, agl1 wrote:
> https://codereview.appspot.com/13528044/diff/6001/ssh/server.go
> File ssh/server.go (right):
> 
> https://codereview.appspot.com/13528044/diff/6001/ssh/server.go#newcode609
> ssh/server.go:609: if !isAcceptableAlgo(algo) || !isAcceptableAlgo(sig.Format)
> || !strings.HasPrefix(algo, sig.Format) {
> On 2013/09/04 18:15:01, jmpittman wrote:
> > I am wondering if the HasPrefix check here makes sense or if a new function
> > should be created to return the private key algorithm name that corresponds
to
> a
> > public key algorithm name.  For most key types, the name will be the same. 
> For
> > certificates, it will differ.  Off the top of my head, I cannot think of any
> > other situations like this.  Thoughts?
> 
> I'm afraid that I've lost what is going on here. (Which probably means that
the
> comment could be expanded.)
> 
> The code is dealing with the cert formats from [1]? And it's checking that an
> "ssh-rsa" signature type is a prefix of "ssh-rsa-cert-v01@openssh.org"? A
prefix
> seems ok, although an explicit function (with comment!) would be better. 
> 
> [1]
>
http://www.openbsd.org/cgi-bin/cvsweb/src/usr.bin/ssh/PROTOCOL.certkeys?rev=1...

Explicit function with comment added.  PTAL

[hanwen-google, 2013-09-05 16:49:30]

https://codereview.appspot.com/13528044/diff/20001/ssh/common.go
File ssh/common.go (right):

https://codereview.appspot.com/13528044/diff/20001/ssh/common.go#newcode214
ssh/common.go:214: func getHashFunc(algo string) (crypto.Hash, bool) {
you could just return nil if you failed.

https://codereview.appspot.com/13528044/diff/20001/ssh/common.go#newcode372
ssh/common.go:372: // pubALgoToPrivAlgo returns the private key algorithm format
name that
typo L -> l

[jmpittman, 2013-09-05 17:11:56]

https://codereview.appspot.com/13528044/diff/20001/ssh/common.go
File ssh/common.go (right):

https://codereview.appspot.com/13528044/diff/20001/ssh/common.go#newcode214
ssh/common.go:214: func getHashFunc(algo string) (crypto.Hash, bool) {
On 2013/09/05 16:49:30, hanwen wrote:
> you could just return nil if you failed.

http://golang.org/pkg/crypto/#Hash indicates that crypto.Has has a base type of
uint.  If it were an interface, a func, or pointer, I might have done just that.
 But using nil results in...

./common.go:225: cannot use nil as type crypto.Hash in return argument

https://codereview.appspot.com/13528044/diff/20001/ssh/common.go#newcode372
ssh/common.go:372: // pubALgoToPrivAlgo returns the private key algorithm format
name that
On 2013/09/05 16:49:30, hanwen wrote:
> typo L -> l 

Done.

[hanwen-google, 2013-09-05 17:24:40]

On Thu, Sep 5, 2013 at 7:11 PM,  <jmpittman@google.com> wrote:
>
> https://codereview.appspot.com/13528044/diff/20001/ssh/common.go
> File ssh/common.go (right):
>
> https://codereview.appspot.com/13528044/diff/20001/ssh/common.go#newcode214
> ssh/common.go:214: func getHashFunc(algo string) (crypto.Hash, bool) {
> On 2013/09/05 16:49:30, hanwen wrote:
>>
>> you could just return nil if you failed.
>
>
> http://golang.org/pkg/crypto/#Hash indicates that crypto.Has has a base
> type of uint.  If it were an interface, a func, or pointer, I might have
> done just that.  But using nil results in...
>
> ./common.go:225: cannot use nil as type crypto.Hash in return argument

ugh - forgot about that.

>
> https://codereview.appspot.com/13528044/diff/20001/ssh/common.go#newcode372
> ssh/common.go:372: // pubALgoToPrivAlgo returns the private key
> algorithm format name that
> On 2013/09/05 16:49:30, hanwen wrote:
>>
>> typo L -> l
>
>
> Done.
>
> https://codereview.appspot.com/13528044/

[jmpittman, 2013-09-05 17:41:57]

On 2013/09/05 17:24:40, hanwen wrote:
> On Thu, Sep 5, 2013 at 7:11 PM,  <mailto:jmpittman@google.com> wrote:
> >
> > https://codereview.appspot.com/13528044/diff/20001/ssh/common.go
> > File ssh/common.go (right):
> >
> > https://codereview.appspot.com/13528044/diff/20001/ssh/common.go#newcode214
> > ssh/common.go:214: func getHashFunc(algo string) (crypto.Hash, bool) {
> > On 2013/09/05 16:49:30, hanwen wrote:
> >>
> >> you could just return nil if you failed.
> >
> >
> > http://golang.org/pkg/crypto/#Hash indicates that crypto.Has has a base
> > type of uint.  If it were an interface, a func, or pointer, I might have
> > done just that.  But using nil results in...
> >
> > ./common.go:225: cannot use nil as type crypto.Hash in return argument
> 
> ugh - forgot about that.
> 
> >
> > https://codereview.appspot.com/13528044/diff/20001/ssh/common.go#newcode372
> > ssh/common.go:372: // pubALgoToPrivAlgo returns the private key
> > algorithm format name that
> > On 2013/09/05 16:49:30, hanwen wrote:
> >>
> >> typo L -> l
> >
> >
> > Done.
> >
> > https://codereview.appspot.com/13528044/

I could do...

func getHashFunc(algo string) (crypto.Hash, bool) {
        var hash crypto.Hash
        switch algo {
        case KeyAlgoRSA, CertAlgoRSAv01, KeyAlgoDSA, CertAlgoDSAv01:
                hash = crypto.SHA1
        case KeyAlgoECDSA256, CertAlgoECDSA256v01:
                hash = crypto.SHA256
        case KeyAlgoECDSA384, CertAlgoECDSA384v01:
                hash = crypto.SHA384
        case KeyAlgoECDSA521, CertAlgoECDSA521v01:
                hash = crypto.SHA512
        }
        return hash, hash.Available()
}

This would be future proof in case the crypto library changed the base type for
crypto.Hash.  Although, hash.Available() failing for a valid hash would be our
fault for not importing the proper package and not so much an issue with an
invalid hash or invalid key algorithm.

[jmpittman, 2013-09-05 19:08:44]

PTAL.

I changed getHashFunc and added a test to hopefully help prevent us from
shipping it out without having the necessary packages imported for the various
hashes.

[hanwen-google, 2013-09-06 07:41:55]

I think the test code (using a map) reads much nicer than the switch code it is
testing, and they say the same thing. 

Without the hashes available, TestKeyExchanges in kex_test would likely fail, so
I wonder if the test adds much value.

Why not simply import the hashes explicitly as you did before? There is just a
limited number of them, and if users of the SSH package were to forget importing
a package, it would be very difficult for them to figure out why things don't
work.

[jmpittman, 2013-09-06 14:21:25]

On 2013/09/06 07:41:55, hanwen wrote:
> I think the test code (using a map) reads much nicer than the switch code it
is
> testing, and they say the same thing. 
> 
Are you then suggesting to use the map inside the function as opposed to the
switch or just use a map instead of the function?  I am thinking the latter.

> Without the hashes available, TestKeyExchanges in kex_test would likely fail,
so
> I wonder if the test adds much value.
>
Fair enough about the value of the test.

 
> Why not simply import the hashes explicitly as you did before? There is just a
> limited number of them, and if users of the SSH package were to forget
importing
> a package, it would be very difficult for them to figure out why things don't
> work.
I am not sure I follow what you mean by "explicitly as you did before."  I have
them imported now with the _ renaming since we only need them for their
initialization to register themselves.

[hanwen-google, 2013-09-06 14:35:41]

LGTM

On Fri, Sep 6, 2013 at 4:21 PM,  <jmpittman@google.com> wrote:
> On 2013/09/06 07:41:55, hanwen wrote:
>>
>> I think the test code (using a map) reads much nicer than the switch
>
> code it is
>>
>> testing, and they say the same thing.
>
>
> Are you then suggesting to use the map inside the function as opposed to
> the switch or just use a map instead of the function?  I am thinking the
> latter.

I guess the latter is a little cleaner (and maybe even faster), but my
main point is that the test looks superfluous.

-- 
Han-Wen Nienhuys
Google Munich
hanwen@google.com

[jmpittman, 2013-09-06 14:50:28]

PTAL

[agl1, 2013-09-09 17:06:22]

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

go.crypto/ssh: Begin adding server side support for more than RSA for client key
auth

R=agl, dave, hanwen
CC=ekg, golang-dev
https://codereview.appspot.com/13528044

Committer: Adam Langley <agl@golang.org>