code review 14540051: go.crypto/ssh: Add certificate verification, step up su...

go.crypto/ssh: Add certificate verification, step up support for authorized keys

[jmpittman, 2013-10-10 03:17:04]

Hello agl1 (cc: dfc, golang-dev@googlegroups.com, hanwen-google, jpsugar),

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

[hanwen-google, 2013-10-10 12:34:24]

This CL has both functionality and cosmetics. Can you put the cosmetic changes
into a different CL ? This will help future readers understand better what is
going on.

https://codereview.appspot.com/14540051/diff/11001/ssh/certs.go
File ssh/certs.go (right):

https://codereview.appspot.com/14540051/diff/11001/ssh/certs.go#newcode58
ssh/certs.go:58: func verifyOpenSSHCertV01(cert *OpenSSHCertV01) bool {
why is this added? It does not seem to be used anywhere.

https://codereview.appspot.com/14540051/diff/11001/ssh/keys_test.go
File ssh/keys_test.go (right):

https://codereview.appspot.com/14540051/diff/11001/ssh/keys_test.go#newcode170
ssh/keys_test.go:170: b64data := []byte("ssh-rsa-cert-v01@openssh.com
AAAAHHNzaC1yc2EtY2VydC12MDFAb3BlbnNzaC5jb20AAAAgb1srW/W3ZDjYAO45xLYAwzHBDLsJ4Ux6ICFIkTjb1LEAAAADAQABAAAAYQCkoR51poH0wE8w72cqSB8Sszx+vAhzcMdCO0wqHTj7UNENHWEXGrU0E0UQekD7U+yhkhtoyjbPOVIP7hNa6aRk/ezdh/iUnCIt4Jt1v3Z1h1P+hA4QuYFMHNB+rmjPwAcAAAAAAAAAAAAAAAEAAAAEdGVzdAAAAAAAAAAAAAAAAP//////////AAAAAAAAAIIAAAAVcGVybWl0LVgxMS1mb3J3YXJkaW5nAAAAAAAAABdwZXJtaXQtYWdlbnQtZm9yd2FyZGluZwAAAAAAAAAWcGVybWl0LXBvcnQtZm9yd2FyZGluZwAAAAAAAAAKcGVybWl0LXB0eQAAAAAAAAAOcGVybWl0LXVzZXItcmMAAAAAAAAAAAAAAHcAAAAHc3NoLXJzYQAAAAMBAAEAAABhANFS2kaktpSGc+CcmEKPyw9mJC4nZKxHKTgLVZeaGbFZOvJTNzBspQHdy7Q1uKSfktxpgjZnksiu/tFF9ngyY2KFoc+U88ya95IZUycBGCUbBQ8+bhDtw/icdDGQD5WnUwAAAG8AAAAHc3NoLXJzYQAAAGC8Y9Z2LQKhIhxf52773XaWrXdxP0t3GBVo4A10vUWiYoAGepr6rQIoGGXFxT4B9Gp+nEBJjOwKDXPrAevow0T9ca8gZN+0ykbhSrXLE5Ao48rqr3zP4O1/9P7e6gp0gw8=")
this data is no longer base64, and the comment no longer accurate.

since the AuthorizedKey code is now completely generic, I don't think it needs
special test treatment for certs.  

Can you make a separate test to provide coverage for verifyOpenSSHCertV01 ? You
can share the key blob between tests.

It seems there is no tests for ParseAuthorizedKey/MarshalAuthorizedKey in this
directory; perhaps we should move the relevant tests from test/ to here.

[jmpittman, 2013-10-10 16:40:00]

https://codereview.appspot.com/14540051/diff/11001/ssh/certs.go
File ssh/certs.go (right):

https://codereview.appspot.com/14540051/diff/11001/ssh/certs.go#newcode58
ssh/certs.go:58: func verifyOpenSSHCertV01(cert *OpenSSHCertV01) bool {
Not yet.  This functionality is going to be needed by the server (and maybe
client?) to verify the Signature belongs with the Cert.  I originally used this
in my changes to TestParseCert, but I have removed it from that test since
parsing and validation are not actually directly related.

https://codereview.appspot.com/14540051/diff/11001/ssh/keys_test.go
File ssh/keys_test.go (right):

https://codereview.appspot.com/14540051/diff/11001/ssh/keys_test.go#newcode170
ssh/keys_test.go:170: b64data := []byte("ssh-rsa-cert-v01@openssh.com
AAAAHHNzaC1yc2EtY2VydC12MDFAb3BlbnNzaC5jb20AAAAgb1srW/W3ZDjYAO45xLYAwzHBDLsJ4Ux6ICFIkTjb1LEAAAADAQABAAAAYQCkoR51poH0wE8w72cqSB8Sszx+vAhzcMdCO0wqHTj7UNENHWEXGrU0E0UQekD7U+yhkhtoyjbPOVIP7hNa6aRk/ezdh/iUnCIt4Jt1v3Z1h1P+hA4QuYFMHNB+rmjPwAcAAAAAAAAAAAAAAAEAAAAEdGVzdAAAAAAAAAAAAAAAAP//////////AAAAAAAAAIIAAAAVcGVybWl0LVgxMS1mb3J3YXJkaW5nAAAAAAAAABdwZXJtaXQtYWdlbnQtZm9yd2FyZGluZwAAAAAAAAAWcGVybWl0LXBvcnQtZm9yd2FyZGluZwAAAAAAAAAKcGVybWl0LXB0eQAAAAAAAAAOcGVybWl0LXVzZXItcmMAAAAAAAAAAAAAAHcAAAAHc3NoLXJzYQAAAAMBAAEAAABhANFS2kaktpSGc+CcmEKPyw9mJC4nZKxHKTgLVZeaGbFZOvJTNzBspQHdy7Q1uKSfktxpgjZnksiu/tFF9ngyY2KFoc+U88ya95IZUycBGCUbBQ8+bhDtw/icdDGQD5WnUwAAAG8AAAAHc3NoLXJzYQAAAGC8Y9Z2LQKhIhxf52773XaWrXdxP0t3GBVo4A10vUWiYoAGepr6rQIoGGXFxT4B9Gp+nEBJjOwKDXPrAevow0T9ca8gZN+0ykbhSrXLE5Ao48rqr3zP4O1/9P7e6gp0gw8=")
Point noted regarding b64data variable name.  Changed.

Why is the comment no longer accurate?

Since the AuthorizedKey code encompasses parsing/marshaling public keys and
certs, do you want just one test for the AuthorizedKey code to handle all of the
supported algorithm cases?  And maybe remove this test and TestKeyMarshalParse? 
Or do you think they need to remain as separate tests?

Test for verifyOpenSSHCertV01 added.

I think the current AuthorizedKey test code was put in test/ to ensure the
functionality works without access to internal package features.  Beyond that
thought, I have no opinion on where it should live and can move it here if there
is a consensus.

[hanwen-google, 2013-10-10 16:56:21]

https://codereview.appspot.com/14540051/diff/11001/ssh/keys_test.go
File ssh/keys_test.go (right):

https://codereview.appspot.com/14540051/diff/11001/ssh/keys_test.go#newcode170
ssh/keys_test.go:170: b64data := []byte("ssh-rsa-cert-v01@openssh.com
AAAAHHNzaC1yc2EtY2VydC12MDFAb3BlbnNzaC5jb20AAAAgb1srW/W3ZDjYAO45xLYAwzHBDLsJ4Ux6ICFIkTjb1LEAAAADAQABAAAAYQCkoR51poH0wE8w72cqSB8Sszx+vAhzcMdCO0wqHTj7UNENHWEXGrU0E0UQekD7U+yhkhtoyjbPOVIP7hNa6aRk/ezdh/iUnCIt4Jt1v3Z1h1P+hA4QuYFMHNB+rmjPwAcAAAAAAAAAAAAAAAEAAAAEdGVzdAAAAAAAAAAAAAAAAP//////////AAAAAAAAAIIAAAAVcGVybWl0LVgxMS1mb3J3YXJkaW5nAAAAAAAAABdwZXJtaXQtYWdlbnQtZm9yd2FyZGluZwAAAAAAAAAWcGVybWl0LXBvcnQtZm9yd2FyZGluZwAAAAAAAAAKcGVybWl0LXB0eQAAAAAAAAAOcGVybWl0LXVzZXItcmMAAAAAAAAAAAAAAHcAAAAHc3NoLXJzYQAAAAMBAAEAAABhANFS2kaktpSGc+CcmEKPyw9mJC4nZKxHKTgLVZeaGbFZOvJTNzBspQHdy7Q1uKSfktxpgjZnksiu/tFF9ngyY2KFoc+U88ya95IZUycBGCUbBQ8+bhDtw/icdDGQD5WnUwAAAG8AAAAHc3NoLXJzYQAAAGC8Y9Z2LQKhIhxf52773XaWrXdxP0t3GBVo4A10vUWiYoAGepr6rQIoGGXFxT4B9Gp+nEBJjOwKDXPrAevow0T9ca8gZN+0ykbhSrXLE5Ao48rqr3zP4O1/9P7e6gp0gw8=")
On 2013/10/10 16:40:00, jmpittman wrote:
> Point noted regarding b64data variable name.  Changed.
> 
> Why is the comment no longer accurate?

If the comment was accurate, the command would not generate the public-key algo
name in front of the blob.

> Since the AuthorizedKey code encompasses parsing/marshaling public keys and
> certs, do you want just one test for the AuthorizedKey code to handle all of
the
> supported algorithm cases?  And maybe remove this test and
TestKeyMarshalParse? 
> Or do you think they need to remain as separate tests?

KeyMarshalParse tests the key-specific routines, so that certainly should be
kept. Notice that it loops through the different key types. It would be nice to
just add a cert key to that list, but that would require a a Signer instance for
certs.

You only have to test the AuthorizedKeys stuff for one key-type if
the key specific code works for all key-types, and that the code for
Marshal/ParseAuthorized key is completely generic. The use of the
acceptableKeyAlgo() function actually breaks that assumption, and it would be
good if things could be written to not need it.

> Test for verifyOpenSSHCertV01 added.
> 
> I think the current AuthorizedKey test code was put in test/ to ensure the
> functionality works without access to internal package features.  Beyond that
> thought, I have no opinion on where it should live and can move it here if
there
> is a consensus.

I have always understood that the test/ package is separate because it fires up
an openssh sshd (which is not available on all platforms.), but I wasn't there
when the decision was made. 

I think that code that can be tested in a self-contained way (eg. verifying that
the Parse/Marshal combo works.) should be together with the package.

BTW, there is a now a code coverage tool for go, 
see http://golang.org/doc/go1.2#cover.  You might want to use it to check what
parts are covered, and which aren't.

[jmpittman, 2013-10-10 18:32:57]

https://codereview.appspot.com/14540051/diff/11001/ssh/keys_test.go
File ssh/keys_test.go (right):

https://codereview.appspot.com/14540051/diff/11001/ssh/keys_test.go#newcode170
ssh/keys_test.go:170: b64data := []byte("ssh-rsa-cert-v01@openssh.com
AAAAHHNzaC1yc2EtY2VydC12MDFAb3BlbnNzaC5jb20AAAAgb1srW/W3ZDjYAO45xLYAwzHBDLsJ4Ux6ICFIkTjb1LEAAAADAQABAAAAYQCkoR51poH0wE8w72cqSB8Sszx+vAhzcMdCO0wqHTj7UNENHWEXGrU0E0UQekD7U+yhkhtoyjbPOVIP7hNa6aRk/ezdh/iUnCIt4Jt1v3Z1h1P+hA4QuYFMHNB+rmjPwAcAAAAAAAAAAAAAAAEAAAAEdGVzdAAAAAAAAAAAAAAAAP//////////AAAAAAAAAIIAAAAVcGVybWl0LVgxMS1mb3J3YXJkaW5nAAAAAAAAABdwZXJtaXQtYWdlbnQtZm9yd2FyZGluZwAAAAAAAAAWcGVybWl0LXBvcnQtZm9yd2FyZGluZwAAAAAAAAAKcGVybWl0LXB0eQAAAAAAAAAOcGVybWl0LXVzZXItcmMAAAAAAAAAAAAAAHcAAAAHc3NoLXJzYQAAAAMBAAEAAABhANFS2kaktpSGc+CcmEKPyw9mJC4nZKxHKTgLVZeaGbFZOvJTNzBspQHdy7Q1uKSfktxpgjZnksiu/tFF9ngyY2KFoc+U88ya95IZUycBGCUbBQ8+bhDtw/icdDGQD5WnUwAAAG8AAAAHc3NoLXJzYQAAAGC8Y9Z2LQKhIhxf52773XaWrXdxP0t3GBVo4A10vUWiYoAGepr6rQIoGGXFxT4B9Gp+nEBJjOwKDXPrAevow0T9ca8gZN+0ykbhSrXLE5Ao48rqr3zP4O1/9P7e6gp0gw8=")
On 2013/10/10 16:56:21, hanwen-google wrote:
> On 2013/10/10 16:40:00, jmpittman wrote:
> > Point noted regarding b64data variable name.  Changed.
> > 
> > Why is the comment no longer accurate?
> 
> If the comment was accurate, the command would not generate the public-key
algo
> name in front of the blob.
> 
In that case, the original comment was likely not completely accurate. 
Ssh-keygen always puts the public key algorithm name in plain text in front of
the blob when it generates the public key or cert.  A trailing comment is often,
but not always, present.  Look in your own "~/.ssh/" for anything ending in .pub
or -cert.pub.

> > Since the AuthorizedKey code encompasses parsing/marshaling public keys and
> > certs, do you want just one test for the AuthorizedKey code to handle all of
> the
> > supported algorithm cases?  And maybe remove this test and
> TestKeyMarshalParse? 
> > Or do you think they need to remain as separate tests?
> 
> KeyMarshalParse tests the key-specific routines, so that certainly should be
> kept. Notice that it loops through the different key types. It would be nice
to
> just add a cert key to that list, but that would require a a Signer instance
for
> certs.
> 

Unfortunately a Signer instance for certs would never exist in the same way that
it does for keys with the current API.  You cannot legally derive a cert from a
single private key.  The signature key is supposed to be different from the main
key in a cert.  (I would like to add a certificate generator though.)

> You only have to test the AuthorizedKeys stuff for one key-type if
> the key specific code works for all key-types, and that the code for
> Marshal/ParseAuthorized key is completely generic. The use of the
> acceptableKeyAlgo() function actually breaks that assumption, and it would be
> good if things could be written to not need it.
> 

My thought was that using the AuthorizedKey code to parse and marshal one
key/cert text for each supported algorithm type would exercise the key/cert
specific routines for parsing and marshaling as well as bring things full circle
from storage format back to storage format.  It could turn a few tests into one
with the same coverage.

> > Test for verifyOpenSSHCertV01 added.
> > 
> > I think the current AuthorizedKey test code was put in test/ to ensure the
> > functionality works without access to internal package features.  Beyond
that
> > thought, I have no opinion on where it should live and can move it here if
> there
> > is a consensus.
> 
> I have always understood that the test/ package is separate because it fires
up
> an openssh sshd (which is not available on all platforms.), but I wasn't there
> when the decision was made. 
> 
> I think that code that can be tested in a self-contained way (eg. verifying
that
> the Parse/Marshal combo works.) should be together with the package.
> 
> BTW, there is a now a code coverage tool for go, 
> see http://golang.org/doc/go1.2#cover.  You might want to use it to check what
> parts are covered, and which aren't.

I was planning to wait for a final 1.2 release, but I just cloned the repo and
will try it.

[jpsugar, 2013-10-10 23:08:17]

https://codereview.appspot.com/14540051/diff/11001/ssh/keys_test.go
File ssh/keys_test.go (right):

https://codereview.appspot.com/14540051/diff/11001/ssh/keys_test.go#newcode170
ssh/keys_test.go:170: b64data := []byte("ssh-rsa-cert-v01@openssh.com
AAAAHHNzaC1yc2EtY2VydC12MDFAb3BlbnNzaC5jb20AAAAgb1srW/W3ZDjYAO45xLYAwzHBDLsJ4Ux6ICFIkTjb1LEAAAADAQABAAAAYQCkoR51poH0wE8w72cqSB8Sszx+vAhzcMdCO0wqHTj7UNENHWEXGrU0E0UQekD7U+yhkhtoyjbPOVIP7hNa6aRk/ezdh/iUnCIt4Jt1v3Z1h1P+hA4QuYFMHNB+rmjPwAcAAAAAAAAAAAAAAAEAAAAEdGVzdAAAAAAAAAAAAAAAAP//////////AAAAAAAAAIIAAAAVcGVybWl0LVgxMS1mb3J3YXJkaW5nAAAAAAAAABdwZXJtaXQtYWdlbnQtZm9yd2FyZGluZwAAAAAAAAAWcGVybWl0LXBvcnQtZm9yd2FyZGluZwAAAAAAAAAKcGVybWl0LXB0eQAAAAAAAAAOcGVybWl0LXVzZXItcmMAAAAAAAAAAAAAAHcAAAAHc3NoLXJzYQAAAAMBAAEAAABhANFS2kaktpSGc+CcmEKPyw9mJC4nZKxHKTgLVZeaGbFZOvJTNzBspQHdy7Q1uKSfktxpgjZnksiu/tFF9ngyY2KFoc+U88ya95IZUycBGCUbBQ8+bhDtw/icdDGQD5WnUwAAAG8AAAAHc3NoLXJzYQAAAGC8Y9Z2LQKhIhxf52773XaWrXdxP0t3GBVo4A10vUWiYoAGepr6rQIoGGXFxT4B9Gp+nEBJjOwKDXPrAevow0T9ca8gZN+0ykbhSrXLE5Ao48rqr3zP4O1/9P7e6gp0gw8=")
On 2013/10/10 18:32:57, jmpittman wrote:
> In that case, the original comment was likely not completely accurate. 

It wasn't. It is now. :)

https://codereview.appspot.com/14540051/diff/11001/ssh/keys_test.go#newcode170
ssh/keys_test.go:170: b64data := []byte("ssh-rsa-cert-v01@openssh.com
AAAAHHNzaC1yc2EtY2VydC12MDFAb3BlbnNzaC5jb20AAAAgb1srW/W3ZDjYAO45xLYAwzHBDLsJ4Ux6ICFIkTjb1LEAAAADAQABAAAAYQCkoR51poH0wE8w72cqSB8Sszx+vAhzcMdCO0wqHTj7UNENHWEXGrU0E0UQekD7U+yhkhtoyjbPOVIP7hNa6aRk/ezdh/iUnCIt4Jt1v3Z1h1P+hA4QuYFMHNB+rmjPwAcAAAAAAAAAAAAAAAEAAAAEdGVzdAAAAAAAAAAAAAAAAP//////////AAAAAAAAAIIAAAAVcGVybWl0LVgxMS1mb3J3YXJkaW5nAAAAAAAAABdwZXJtaXQtYWdlbnQtZm9yd2FyZGluZwAAAAAAAAAWcGVybWl0LXBvcnQtZm9yd2FyZGluZwAAAAAAAAAKcGVybWl0LXB0eQAAAAAAAAAOcGVybWl0LXVzZXItcmMAAAAAAAAAAAAAAHcAAAAHc3NoLXJzYQAAAAMBAAEAAABhANFS2kaktpSGc+CcmEKPyw9mJC4nZKxHKTgLVZeaGbFZOvJTNzBspQHdy7Q1uKSfktxpgjZnksiu/tFF9ngyY2KFoc+U88ya95IZUycBGCUbBQ8+bhDtw/icdDGQD5WnUwAAAG8AAAAHc3NoLXJzYQAAAGC8Y9Z2LQKhIhxf52773XaWrXdxP0t3GBVo4A10vUWiYoAGepr6rQIoGGXFxT4B9Gp+nEBJjOwKDXPrAevow0T9ca8gZN+0ykbhSrXLE5Ao48rqr3zP4O1/9P7e6gp0gw8=")
On 2013/10/10 18:32:57, jmpittman wrote:
> Unfortunately a Signer instance for certs would never exist in the same way
that
> it does for keys with the current API.

I don't see why not. Nobody expects the Signer to perform CA functions -- that's
generally the purview of a different entity. :)

[jmpittman, 2013-10-11 16:13:42]

https://codereview.appspot.com/14540051/diff/11001/ssh/keys_test.go
File ssh/keys_test.go (right):

https://codereview.appspot.com/14540051/diff/11001/ssh/keys_test.go#newcode170
ssh/keys_test.go:170: b64data := []byte("ssh-rsa-cert-v01@openssh.com
AAAAHHNzaC1yc2EtY2VydC12MDFAb3BlbnNzaC5jb20AAAAgb1srW/W3ZDjYAO45xLYAwzHBDLsJ4Ux6ICFIkTjb1LEAAAADAQABAAAAYQCkoR51poH0wE8w72cqSB8Sszx+vAhzcMdCO0wqHTj7UNENHWEXGrU0E0UQekD7U+yhkhtoyjbPOVIP7hNa6aRk/ezdh/iUnCIt4Jt1v3Z1h1P+hA4QuYFMHNB+rmjPwAcAAAAAAAAAAAAAAAEAAAAEdGVzdAAAAAAAAAAAAAAAAP//////////AAAAAAAAAIIAAAAVcGVybWl0LVgxMS1mb3J3YXJkaW5nAAAAAAAAABdwZXJtaXQtYWdlbnQtZm9yd2FyZGluZwAAAAAAAAAWcGVybWl0LXBvcnQtZm9yd2FyZGluZwAAAAAAAAAKcGVybWl0LXB0eQAAAAAAAAAOcGVybWl0LXVzZXItcmMAAAAAAAAAAAAAAHcAAAAHc3NoLXJzYQAAAAMBAAEAAABhANFS2kaktpSGc+CcmEKPyw9mJC4nZKxHKTgLVZeaGbFZOvJTNzBspQHdy7Q1uKSfktxpgjZnksiu/tFF9ngyY2KFoc+U88ya95IZUycBGCUbBQ8+bhDtw/icdDGQD5WnUwAAAG8AAAAHc3NoLXJzYQAAAGC8Y9Z2LQKhIhxf52773XaWrXdxP0t3GBVo4A10vUWiYoAGepr6rQIoGGXFxT4B9Gp+nEBJjOwKDXPrAevow0T9ca8gZN+0ykbhSrXLE5Ao48rqr3zP4O1/9P7e6gp0gw8=")
On 2013/10/10 23:08:17, jpsugar wrote:
> On 2013/10/10 18:32:57, jmpittman wrote:
> > Unfortunately a Signer instance for certs would never exist in the same way
> that
> > it does for keys with the current API.
> 
> I don't see why not. Nobody expects the Signer to perform CA functions --
that's
> generally the purview of a different entity. :)

It can be done, but the implementation would not be as natural as a private key
being a Signer that returns the plain public key.  A private key does not
inherently contain a certificate.  The cert has to be created and signed by
another private key.  You would need to wrap the private key and the cert in a
struct that will divide up the Signer responsibilities between the two.  You
kind of have to hack it together unless you can rely on something like an ssh
agent or other key ring.

I added an implementation and added it to TestKeyMarshalParse and
TestKeySignVerify.  The tests pass in TestKeySignVerify.  I am working to get it
to pass the reflect.DeepEqual call in TestKeyMarshalParse.

[jmpittman, 2013-10-11 20:30:42]

On 2013/10/11 16:13:42, jmpittman wrote:
> https://codereview.appspot.com/14540051/diff/11001/ssh/keys_test.go
> File ssh/keys_test.go (right):
> 
> https://codereview.appspot.com/14540051/diff/11001/ssh/keys_test.go#newcode170
> ssh/keys_test.go:170: b64data := mailto:[]byte("ssh-rsa-cert-v01@openssh.com
>
AAAAHHNzaC1yc2EtY2VydC12MDFAb3BlbnNzaC5jb20AAAAgb1srW/W3ZDjYAO45xLYAwzHBDLsJ4Ux6ICFIkTjb1LEAAAADAQABAAAAYQCkoR51poH0wE8w72cqSB8Sszx+vAhzcMdCO0wqHTj7UNENHWEXGrU0E0UQekD7U+yhkhtoyjbPOVIP7hNa6aRk/ezdh/iUnCIt4Jt1v3Z1h1P+hA4QuYFMHNB+rmjPwAcAAAAAAAAAAAAAAAEAAAAEdGVzdAAAAAAAAAAAAAAAAP//////////AAAAAAAAAIIAAAAVcGVybWl0LVgxMS1mb3J3YXJkaW5nAAAAAAAAABdwZXJtaXQtYWdlbnQtZm9yd2FyZGluZwAAAAAAAAAWcGVybWl0LXBvcnQtZm9yd2FyZGluZwAAAAAAAAAKcGVybWl0LXB0eQAAAAAAAAAOcGVybWl0LXVzZXItcmMAAAAAAAAAAAAAAHcAAAAHc3NoLXJzYQAAAAMBAAEAAABhANFS2kaktpSGc+CcmEKPyw9mJC4nZKxHKTgLVZeaGbFZOvJTNzBspQHdy7Q1uKSfktxpgjZnksiu/tFF9ngyY2KFoc+U88ya95IZUycBGCUbBQ8+bhDtw/icdDGQD5WnUwAAAG8AAAAHc3NoLXJzYQAAAGC8Y9Z2LQKhIhxf52773XaWrXdxP0t3GBVo4A10vUWiYoAGepr6rQIoGGXFxT4B9Gp+nEBJjOwKDXPrAevow0T9ca8gZN+0ykbhSrXLE5Ao48rqr3zP4O1/9P7e6gp0gw8=")
> On 2013/10/10 23:08:17, jpsugar wrote:
> > On 2013/10/10 18:32:57, jmpittman wrote:
> > > Unfortunately a Signer instance for certs would never exist in the same
way
> > that
> > > it does for keys with the current API.
> > 
> > I don't see why not. Nobody expects the Signer to perform CA functions --
> that's
> > generally the purview of a different entity. :)
> 
> It can be done, but the implementation would not be as natural as a private
key
> being a Signer that returns the plain public key.  A private key does not
> inherently contain a certificate.  The cert has to be created and signed by
> another private key.  You would need to wrap the private key and the cert in a
> struct that will divide up the Signer responsibilities between the two.  You
> kind of have to hack it together unless you can rely on something like an ssh
> agent or other key ring.
> 
> I added an implementation and added it to TestKeyMarshalParse and
> TestKeySignVerify.  The tests pass in TestKeySignVerify.  I am working to get
it
> to pass the reflect.DeepEqual call in TestKeyMarshalParse.

I have the tests passing, but I ran into an issue.  I am not sure if the problem
lies with the behavior of parseString, marshalString, or reflect.DeepEqual.  See
this link for an example:

http://play.golang.org/p/AJ5sXvUBfi

Toggle the comment between the first two lines in main.  It boils down to a nil
slice and an empty slice of the same type do not pass reflect.DeepEqual.  This
could be accounted for by modifying parseString, but I am not sure if it should.
 Thoughts?

I added a few items to increase test coverage for functions dealing with ecdsa
and some of the parsing/marshaling functions.

[jmpittman, 2013-10-11 20:50:41]

https://codereview.appspot.com/14540051/diff/25001/ssh/certs.go
File ssh/certs.go (right):

https://codereview.appspot.com/14540051/diff/25001/ssh/certs.go#newcode160
ssh/certs.go:160: if cert.Type, in, ok = parseUint32(in); !ok {
Reason for the change to this line...  There were two issues with this.

1. It was broken.  If ok was true, but the cert.Type checks did not pass, the
function would return, but ok was not set to false before returning.  The
returned cert value would be nil and the calling functions would assume it
passed.
2. The cert.Type checks were really a validation that should occur somewhere
outside of this function.

[hanwen-google, 2013-10-12 09:53:58]

https://codereview.appspot.com/14540051/diff/25001/ssh/certs.go
File ssh/certs.go (right):

https://codereview.appspot.com/14540051/diff/25001/ssh/certs.go#newcode160
ssh/certs.go:160: if cert.Type, in, ok = parseUint32(in); !ok {
On 2013/10/11 20:50:41, jmpittman wrote:
> Reason for the change to this line...  There were two issues with this.
> 
> 1. It was broken.  If ok was true, but the cert.Type checks did not pass, the
> function would return, but ok was not set to false before returning.  The
> returned cert value would be nil and the calling functions would assume it
> passed.
> 2. The cert.Type checks were really a validation that should occur somewhere
> outside of this function.

do we have test that would catch this if this problem were reintroduced?

https://codereview.appspot.com/14540051/diff/25001/ssh/keys.go
File ssh/keys.go (right):

https://codereview.appspot.com/14540051/diff/25001/ssh/keys.go#newcode78
ssh/keys.go:78: func ParseAuthorizedKey(in []byte) (out PublicKey, comment
string, options []string, rest []byte, ok bool) {
I wonder whether this should return error insteda of bool. There could be useful
diagnostics for the parse failure.

https://codereview.appspot.com/14540051/diff/25001/ssh/keys.go#newcode109
ssh/keys.go:109: }
instead of checking the type, could you just run parseAuthorizedKeys, and if it
does not return success continue? the same for the last call to
parseAuthorizedKeys.

https://codereview.appspot.com/14540051/diff/25001/ssh/keys_test.go
File ssh/keys_test.go (right):

https://codereview.appspot.com/14540051/diff/25001/ssh/keys_test.go#newcode55
ssh/keys_test.go:55: keys := []Signer{rsaKey, dsaKey, ecdsaKey, ecdsa384Key,
ecdsa521Key, testCertKey}
yes, this looks much nicer.

[hanwen-google, 2013-10-12 09:54:13]

On Fri, Oct 11, 2013 at 10:30 PM,  <jmpittman@google.com> wrote:
> I have the tests passing, but I ran into an issue.  I am not sure if the
> problem lies with the behavior of parseString, marshalString, or
> reflect.DeepEqual.  See this link for an example:
>
> http://play.golang.org/p/AJ5sXvUBfi
>
> Toggle the comment between the first two lines in main.  It boils down
> to a nil slice and an empty slice of the same type do not pass
> reflect.DeepEqual.  This could be accounted for by modifying
> parseString, but I am not sure if it should.  Thoughts?

where does the problem trigger exactly? nil and empty are different
for reflect.

One way to tackle this is to test the roundtrip bytes -> parse ->
marshal -> bytes

and then rely on byte equality.

> I added a few items to increase test coverage for functions dealing with
> ecdsa and some of the parsing/marshaling functions.
>
> https://codereview.appspot.com/14540051/

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

[jmpittman, 2013-10-14 16:44:15]

https://codereview.appspot.com/14540051/diff/25001/ssh/certs.go
File ssh/certs.go (right):

https://codereview.appspot.com/14540051/diff/25001/ssh/certs.go#newcode160
ssh/certs.go:160: if cert.Type, in, ok = parseUint32(in); !ok {
On 2013/10/12 09:53:58, hanwen-google wrote:
> On 2013/10/11 20:50:41, jmpittman wrote:
> > Reason for the change to this line...  There were two issues with this.
> > 
> > 1. It was broken.  If ok was true, but the cert.Type checks did not pass,
the
> > function would return, but ok was not set to false before returning.  The
> > returned cert value would be nil and the calling functions would assume it
> > passed.
> > 2. The cert.Type checks were really a validation that should occur somewhere
> > outside of this function.
> 
> do we have test that would catch this if this problem were reintroduced?

I have added a certs_test.go file and have added tests that cover the majority
of failure cases for this function.  One specific section not covered for a
failure is...

lines 151-153: if cert.Key.PublicKeyAlgo() != privAlgo {

I am not certain how to force that to fail.  It may be that this is a validation
that can be moved outside of the parsing function.

The original issue should not come up again unless that check is added back
here.  And if it is added, the new test should be modified to handle the
scenario.

https://codereview.appspot.com/14540051/diff/25001/ssh/keys.go
File ssh/keys.go (right):

https://codereview.appspot.com/14540051/diff/25001/ssh/keys.go#newcode78
ssh/keys.go:78: func ParseAuthorizedKey(in []byte) (out PublicKey, comment
string, options []string, rest []byte, ok bool) {
On 2013/10/12 09:53:58, hanwen-google wrote:
> I wonder whether this should return error insteda of bool. There could be
useful
> diagnostics for the parse failure.

There are a lot of places in this package where I wonder the same thing.  Save
for another CL?  I wanted to add the basic support for the other key and cert
types.  They might have worked all along, but were left out for some reason.

https://codereview.appspot.com/14540051/diff/25001/ssh/keys.go#newcode109
ssh/keys.go:109: }
On 2013/10/12 09:53:58, hanwen-google wrote:
> instead of checking the type, could you just run parseAuthorizedKeys, and if
it
> does not return success continue? the same for the last call to
> parseAuthorizedKeys.
> 

Done.

[jmpittman, 2013-10-14 16:52:29]

On 2013/10/12 09:54:13, hanwen-google wrote:
> On Fri, Oct 11, 2013 at 10:30 PM,  <mailto:jmpittman@google.com> wrote:
> > I have the tests passing, but I ran into an issue.  I am not sure if the
> > problem lies with the behavior of parseString, marshalString, or
> > reflect.DeepEqual.  See this link for an example:
> >
> > http://play.golang.org/p/AJ5sXvUBfi
> >
> > Toggle the comment between the first two lines in main.  It boils down
> > to a nil slice and an empty slice of the same type do not pass
> > reflect.DeepEqual.  This could be accounted for by modifying
> > parseString, but I am not sure if it should.  Thoughts?
> 
> where does the problem trigger exactly? nil and empty are different
> for reflect.
The marshalString will take either an empty slice or a nil slice and create a
slice of 4 bytes containing all 0 (for the length).  When that gets run through
parseString, it will return an empty slice, but not a nil slice.

> 
> One way to tackle this is to test the roundtrip bytes -> parse ->
> marshal -> bytes
> 
> and then rely on byte equality.
> 

Agreed.  I see a few options:

1. Leave everything as is and just always know that we must use a non-nil slice
before marshaling.
2. Take testCertKey back out of TestKeyMarshalParse and just use TestParseCert
like was done originally.
3. Change TestKeyMarshalParse to parse some externally generated key strings and
then marshal them back to bytes.
4. Have two parse/marshal tests where one roundtrips from struct to bytes to
struct and the other roundtrips from bytes to struct to bytes.

Any preference?

> > I added a few items to increase test coverage for functions dealing with
> > ecdsa and some of the parsing/marshaling functions.
> >
> > https://codereview.appspot.com/14540051/
> 
> -- 
> Han-Wen Nienhuys
> Google Munich
> mailto:hanwen@google.com

[hanwen-google, 2013-10-15 17:27:42]

On Mon, Oct 14, 2013 at 9:52 AM,  <jmpittman@google.com> wrote:
>
>
> Agreed.  I see a few options:
>

A simple solution would be to set the slice fields to non-nil in
testCert. Then the current test would work? You can add a comment to
explain why this is necessary.


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

[jmpittman, 2013-10-15 17:35:42]

On 2013/10/15 17:27:42, hanwen-google wrote:
> On Mon, Oct 14, 2013 at 9:52 AM,  <mailto:jmpittman@google.com> wrote:
> >
> >
> > Agreed.  I see a few options:
> >
> 
> A simple solution would be to set the slice fields to non-nil in
> testCert. Then the current test would work? You can add a comment to
> explain why this is necessary.
> 
> 
> -- 
> Han-Wen Nienhuys
> Google Munich
> mailto:hanwen@google.com

I have them set to non-nil now.  I will add a comment.

[hanwen-google, 2013-10-15 17:52:55]

https://codereview.appspot.com/14540051/diff/46001/ssh/certs_test.go
File ssh/certs_test.go (right):

https://codereview.appspot.com/14540051/diff/46001/ssh/certs_test.go#newcode158
ssh/certs_test.go:158: t.Error("Cert parsing passed with incomplete pub key
bytes!")
Isn't parseOpenSSHCert() composed of functions that fail on their own? I think
it would be more useful to have unittests for the functions you use rather than
these tests. As a bonus, if somebody introduces an error, it would point to the
function at fault.  

If you misparse a cert, the signature bytes wouldn't be correct, so the cert
wouldn't validate. Doesn't that provide enough assurance against bugs?

[jmpittman, 2013-10-15 18:57:30]

On 2013/10/15 17:52:55, hanwen-google wrote:
> https://codereview.appspot.com/14540051/diff/46001/ssh/certs_test.go
> File ssh/certs_test.go (right):
> 
>
https://codereview.appspot.com/14540051/diff/46001/ssh/certs_test.go#newcode158
> ssh/certs_test.go:158: t.Error("Cert parsing passed with incomplete pub key
> bytes!")
> Isn't parseOpenSSHCert() composed of functions that fail on their own? I think
> it would be more useful to have unittests for the functions you use rather
than
> these tests. As a bonus, if somebody introduces an error, it would point to
the
> function at fault.  
> 
> If you misparse a cert, the signature bytes wouldn't be correct, so the cert
> wouldn't validate. Doesn't that provide enough assurance against bugs?

With the current state of parseOpenSSHCert and the signature validation, I think
it should provide enough assurance against bugs.  I can rework this to provide
test cases for the individual functions.  Then I will see if I can still break
it via other means.

[jmpittman, 2013-10-16 20:39:14]

On second thought, I would like to remove the test for parseOpenSSHCert as you
suggested and just have the rest of this submitted as is if all looks okay. 
Then I want to follow up with a CL that will rearrange all of the lower level
(string, int, etc) parse/marshal/length functions into a utils.go file and then
add tests for those functions.  There is much that is spread around in various
files, but could be consolidated.  Adding tests for these individual parsing and
marshaling functions will go beyond the intent of this CL.

How does that sound?

[hanwen-google, 2013-10-16 20:40:23]

On 2013/10/16 20:39:14, jmpittman wrote:
> On second thought, I would like to remove the test for parseOpenSSHCert as you
> suggested and just have the rest of this submitted as is if all looks okay. 
> Then I want to follow up with a CL that will rearrange all of the lower level
> (string, int, etc) parse/marshal/length functions into a utils.go file and
then
> add tests for those functions.  There is much that is spread around in various
> files, but could be consolidated.  Adding tests for these individual parsing
and
> marshaling functions will go beyond the intent of this CL.
> 
> How does that sound?

SGTM

[jmpittman, 2013-10-16 20:43:40]

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

Please take another look.

[hanwen-google, 2013-10-16 20:45:23]

https://codereview.appspot.com/14540051/diff/57001/ssh/certs_test.go
File ssh/certs_test.go (right):

https://codereview.appspot.com/14540051/diff/57001/ssh/certs_test.go#newcode16
ssh/certs_test.go:16: /* Structure of the base64 section of exampleSSHCert is as
follows:
this can go now?

[jmpittman, 2013-10-16 20:51:07]

https://codereview.appspot.com/14540051/diff/57001/ssh/certs_test.go
File ssh/certs_test.go (right):

https://codereview.appspot.com/14540051/diff/57001/ssh/certs_test.go#newcode16
ssh/certs_test.go:16: /* Structure of the base64 section of exampleSSHCert is as
follows:
On 2013/10/16 20:45:24, hanwen-google wrote:
> this can go now?

Done.

[hanwen-google, 2013-10-17 00:48:55]

LGTM

[jmpittman, 2013-10-17 22:46:34]

Can I get dfc or agl to take a look at this?

[dfc, 2013-10-19 11:24:23]

Thanks. Code LGTM. I've added a bunch of unhelpful nits to the review. 

I really don't feel qualified to comment on the way the certificates are
handled, this is really up to agl, but I think he'll be happy.

https://codereview.appspot.com/14540051/diff/65001/ssh/certs.go
File ssh/certs.go (right):

https://codereview.appspot.com/14540051/diff/65001/ssh/certs.go#newcode89
ssh/certs.go:89: length += 8 // Length of Serial
this is probably bike shedding but what about making these values constants like

serialLength and typeLength (you could even do them at the top of the function,
they needn't be package level) then you can do

length += serialLegnth + typeLength.

https://codereview.appspot.com/14540051/diff/65001/ssh/certs.go#newcode93
ssh/certs.go:93: length += 8 // Length of ValidAfter
same

https://codereview.appspot.com/14540051/diff/65001/ssh/certs.go#newcode116
ssh/certs.go:116: panic("internal error")
panic("ssh: internal error, marshalling certificate did not fill the entire
buffer") // or something.

https://codereview.appspot.com/14540051/diff/65001/ssh/keys_test.go
File ssh/keys_test.go (right):

https://codereview.appspot.com/14540051/diff/65001/ssh/keys_test.go#newcode24
ssh/keys_test.go:24: priv Signer
Signer

https://codereview.appspot.com/14540051/diff/65001/ssh/keys_test.go#newcode32
ssh/keys_test.go:32: return ts.priv.PublicKey()
return ts.Signer.PublicKey()

https://codereview.appspot.com/14540051/diff/65001/ssh/keys_test.go#newcode36
ssh/keys_test.go:36: return ts.priv.Sign(rand, data)
then you can delete this forwarding method

https://codereview.appspot.com/14540051/diff/65001/ssh/keys_test.go#newcode191
ssh/keys_test.go:191: func init() {
please move the init to the top of the file

[jmpittman, 2013-10-20 05:17:21]

https://codereview.appspot.com/14540051/diff/65001/ssh/certs.go
File ssh/certs.go (right):

https://codereview.appspot.com/14540051/diff/65001/ssh/certs.go#newcode89
ssh/certs.go:89: length += 8 // Length of Serial
On 2013/10/19 11:24:24, dfc wrote:
> this is probably bike shedding but what about making these values constants
like
> 
> serialLength and typeLength (you could even do them at the top of the
function,
> they needn't be package level) then you can do
> 
> length += serialLegnth + typeLength.

This code is mostly copied and then modified from the Marshal() method below. 
Originally, I was running Marshal, subtracting the signature bytes from the end,
and adding the algo name to the beginning.  I chose to rewrite it here because
that felt very hacked together and less understandable instead of being
straightforward like this.  Also, the signature will not always be present when
this method gets called.  This method will be used for cert generation (no
signature present) as well as validation (signature present).

If the changes are desired to improve readability in both method calls, I have
no objection to making them.

https://codereview.appspot.com/14540051/diff/65001/ssh/certs.go#newcode93
ssh/certs.go:93: length += 8 // Length of ValidAfter
On 2013/10/19 11:24:24, dfc wrote:
> same

ditto

https://codereview.appspot.com/14540051/diff/65001/ssh/certs.go#newcode116
ssh/certs.go:116: panic("internal error")
On 2013/10/19 11:24:24, dfc wrote:
> panic("ssh: internal error, marshalling certificate did not fill the entire
> buffer") // or something.

Done.  And changed below.  Much nicer.

https://codereview.appspot.com/14540051/diff/65001/ssh/keys_test.go
File ssh/keys_test.go (right):

https://codereview.appspot.com/14540051/diff/65001/ssh/keys_test.go#newcode24
ssh/keys_test.go:24: priv Signer
On 2013/10/19 11:24:24, dfc wrote:
> Signer

Done.

https://codereview.appspot.com/14540051/diff/65001/ssh/keys_test.go#newcode32
ssh/keys_test.go:32: return ts.priv.PublicKey()
On 2013/10/19 11:24:24, dfc wrote:
> return ts.Signer.PublicKey()

Done.

https://codereview.appspot.com/14540051/diff/65001/ssh/keys_test.go#newcode36
ssh/keys_test.go:36: return ts.priv.Sign(rand, data)
On 2013/10/19 11:24:24, dfc wrote:
> then you can delete this forwarding method

Done.

https://codereview.appspot.com/14540051/diff/65001/ssh/keys_test.go#newcode191
ssh/keys_test.go:191: func init() {
On 2013/10/19 11:24:24, dfc wrote:
> please move the init to the top of the file

Done.

[hanwen-google, 2013-10-20 05:23:21]

On Sat, Oct 19, 2013 at 10:17 PM,  <jmpittman@google.com> wrote:
> This code is mostly copied and then modified from the Marshal() method
> below.  Originally, I was running Marshal, subtracting the signature

I didn't notice this, sorry. Could you not make a method taking bools
whether or not to include algo name, and the signature, and use that
in both Marshal and BytesForSig ?



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

[jmpittman, 2013-10-20 05:29:22]

Yeah, I was considering some kind of "core" marshaling method that would add the
necessary bits on the front or back end.  I will rework it as an unexported
method and then simplify these two.

[jmpittman, 2013-10-20 05:45:40]

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

Please take another look.

[jmpittman, 2013-10-22 16:49:00]

Ping.

[jpsugar, 2013-10-22 17:01:45]

LGTM with nits

https://codereview.appspot.com/14540051/diff/148001/ssh/certs.go
File ssh/certs.go (right):

https://codereview.appspot.com/14540051/diff/148001/ssh/certs.go#newcode55
ssh/certs.go:55: // validateOpenSSHCertV01Signature uses the cert's SignatureKey
to verify that the
Re-wrap comment please.

https://codereview.appspot.com/14540051/diff/148001/ssh/certs.go#newcode98
ssh/certs.go:98: if includeAlgo {
I would prefer for this list to be in the same order as the list below.

[jmpittman, 2013-10-22 18:52:16]

PTAL jpsugar.  If all is well, agl, I am ready.

https://codereview.appspot.com/14540051/diff/148001/ssh/certs.go
File ssh/certs.go (right):

https://codereview.appspot.com/14540051/diff/148001/ssh/certs.go#newcode55
ssh/certs.go:55: // validateOpenSSHCertV01Signature uses the cert's SignatureKey
to verify that the
Wrapped to 80.

https://codereview.appspot.com/14540051/diff/148001/ssh/certs.go#newcode98
ssh/certs.go:98: if includeAlgo {
The order does not matter for the length, but it does help readability.  I only
did this so I wouldn't have to do "var length int" up above.  Changed.

[jpsugar, 2013-10-22 18:54:31]

Something is horribly wrong with that upload. I get "old chunk mismatch" trying
to view diffs.

[agl1, 2013-10-22 18:55:33]

Yea, the upload went wonky.

[jmpittman, 2013-10-22 18:55:55]

Yeah, weird.  I uploaded again.  Try it now.

[jpsugar, 2013-10-22 19:05:47]

LGTM

[agl1, 2013-10-22 19:12:56]

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

go.crypto/ssh: Add certificate verification, step up support for authorized keys

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

Committer: Adam Langley <agl@golang.org>