Code review - Issue 14540051: code review 14540051: go.crypto/ssh: Add certificate verification, step up su...https://codereview.appspot.com/2013-10-22T19:12:56+00:00rietveld
Message from unknown
2013-10-10T02:59:04+00:00jmpittmanurn:md5:c0ca29fe7406545fe383f7fe2f628718
Message from unknown
2013-10-10T02:59:08+00:00jmpittmanurn:md5:68baa4b63b8c2f6341e94377deefd084
Message from unknown
2013-10-10T03:06:51+00:00jmpittmanurn:md5:f25157d0557836df2cb516b0f959bd62
Message from unknown
2013-10-10T03:14:41+00:00jmpittmanurn:md5:55f9fce42ada3fc339f36d5814fff4bb
Message from unknown
2013-10-10T03:16:58+00:00jmpittmanurn:md5:ec39965e35adc5615abd0d4f4cb43086
Message from jmpittman@google.com
2013-10-10T03:17:04+00:00jmpittmanurn:md5:e7a62ffff8d78aa3dc033afb8be0b7c7
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
Message from hanwen@google.com
2013-10-10T12:34:24+00:00hanwen-googleurn:md5:e40f99ddab3cc91c3b7a632af0b8c86f
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.
Message from unknown
2013-10-10T14:37:26+00:00jmpittmanurn:md5:6f184be3e6a5fafb12cd14701268d6ac
Message from unknown
2013-10-10T16:32:31+00:00jmpittmanurn:md5:365664c9ad9f46c1a822b7eb55cef977
Message from unknown
2013-10-10T16:34:02+00:00jmpittmanurn:md5:ddcf9446b20758466407628bc7a31b2d
Message from unknown
2013-10-10T16:36:45+00:00jmpittmanurn:md5:e7731799f79c34f06400d55335905438
Message from unknown
2013-10-10T16:37:48+00:00jmpittmanurn:md5:60b7a46492d4129e3d56b50711819923
Message from jmpittman@google.com
2013-10-10T16:40:00+00:00jmpittmanurn:md5:3e1db1bcb95e33ae7f29bb610c5b271d
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.
Message from hanwen@google.com
2013-10-10T16:56:21+00:00hanwen-googleurn:md5:aa96ef8fdb7cea0a63f74fea2ec499e3
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.
Message from jmpittman@google.com
2013-10-10T18:32:57+00:00jmpittmanurn:md5:f2ce298a1900b1772ad40d6b04f333c1
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.
Message from jpsugar@google.com
2013-10-10T23:08:17+00:00jpsugarurn:md5:b85c7b9319236265e822e1752ec0c36c
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. :)
Message from jmpittman@google.com
2013-10-11T16:13:42+00:00jmpittmanurn:md5:3501fb64b6138a881cb79f5332e496b0
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.
Message from unknown
2013-10-11T20:23:09+00:00jmpittmanurn:md5:d4e5ccf6972dff7205f1291117c51505
Message from jmpittman@google.com
2013-10-11T20:30:42+00:00jmpittmanurn:md5:621047f47c00353615e85d522111de77
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.
Message from jmpittman@google.com
2013-10-11T20:50:41+00:00jmpittmanurn:md5:be57ed18c9c96e32faa47aab5cb951aa
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.
Message from hanwen@google.com
2013-10-12T09:53:58+00:00hanwen-googleurn:md5:50a22e1475a7ecabfd9fec1f1c27cf5b
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.
Message from hanwen@google.com
2013-10-12T09:54:13+00:00hanwen-googleurn:md5:5f9d7aa96a8a56d875efb23aa332a403
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
Message from unknown
2013-10-14T16:39:44+00:00jmpittmanurn:md5:4217c83f9caef663b91dfe7cb5201af4
Message from jmpittman@google.com
2013-10-14T16:44:15+00:00jmpittmanurn:md5:1561d98275c6e36f434c009eb42ae9f6
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.
Message from jmpittman@google.com
2013-10-14T16:52:29+00:00jmpittmanurn:md5:0407cdc37aefe81dfd7f322ca327222e
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
Message from unknown
2013-10-14T16:56:04+00:00jmpittmanurn:md5:1a1902c5ef284defb2ff3370935d5401
Message from unknown
2013-10-14T17:57:20+00:00jmpittmanurn:md5:bb54f0c5dde583a73a5c0d49c78be2bf
Message from hanwen@google.com
2013-10-15T17:27:42+00:00hanwen-googleurn:md5:c0ee8deac40ae58cd0bbca7a02bef59e
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
Message from jmpittman@google.com
2013-10-15T17:35:42+00:00jmpittmanurn:md5:3ab586ae9e04e3a61bc6aa8c5652d823
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.
Message from hanwen@google.com
2013-10-15T17:52:55+00:00hanwen-googleurn:md5:b2abc3ff20326cde235f9b68f259c3ca
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?
Message from jmpittman@google.com
2013-10-15T18:57:30+00:00jmpittmanurn:md5:9e875c91cac050ad20c4939b9831ac1c
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.
Message from jmpittman@google.com
2013-10-16T20:39:14+00:00jmpittmanurn:md5:d16349308c37687f00dd110567da6e7e
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?
Message from hanwen@google.com
2013-10-16T20:40:23+00:00hanwen-googleurn:md5:3761a505270857e95e1bdbca7671e027
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
Message from unknown
2013-10-16T20:43:20+00:00jmpittmanurn:md5:5e32de133cba43d807fb0d014554cf51
Message from unknown
2013-10-16T20:43:35+00:00jmpittmanurn:md5:25f74981638da272e094943f572984a0
Message from jmpittman@google.com
2013-10-16T20:43:40+00:00jmpittmanurn:md5:703d6ab29c412843f334b93fef20ce8a
Hello agl@golang.org, hanwen@google.com, jpsugar@google.com (cc: dave@cheney.net, golang-dev@googlegroups.com),
Please take another look.
Message from hanwen@google.com
2013-10-16T20:45:23+00:00hanwen-googleurn:md5:27820d305e785eaaf189a04da4d5e880
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?
Message from unknown
2013-10-16T20:50:16+00:00jmpittmanurn:md5:31b88a4c3350b01040f93d8b42f513b7
Message from jmpittman@google.com
2013-10-16T20:51:07+00:00jmpittmanurn:md5:e045b9a599bf14b8281dcfcc8be585b6
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.
Message from hanwen@google.com
2013-10-17T00:48:55+00:00hanwen-googleurn:md5:ec34f7ce3d47a4ef142b81d27f60316b
LGTM
Message from jmpittman@google.com
2013-10-17T22:46:34+00:00jmpittmanurn:md5:1d87b6749b539c3608b5965b355b1a6f
Can I get dfc or agl to take a look at this?
Message from dave@cheney.net
2013-10-19T11:24:23+00:00dfcurn:md5:cf0ca2db7ce46a563a8680b71a4207d1
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
Message from unknown
2013-10-20T04:57:21+00:00jmpittmanurn:md5:cb2c8f4b4f0c843b233bab0f5c3d4b16
Message from jmpittman@google.com
2013-10-20T05:17:21+00:00jmpittmanurn:md5:b6413324e811853ea25e5306f63d7400
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.
Message from hanwen@google.com
2013-10-20T05:23:21+00:00hanwen-googleurn:md5:87cf070a22f8a341c4cd24c421215635
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
Message from jmpittman@google.com
2013-10-20T05:29:22+00:00jmpittmanurn:md5:9e802229822209782a43c35ba9a1117b
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.
Message from unknown
2013-10-20T05:45:34+00:00jmpittmanurn:md5:dee65e933639279ca23dcb2dc7a17cb9
Message from jmpittman@google.com
2013-10-20T05:45:40+00:00jmpittmanurn:md5:4e8dfdc4ccc0f2fd0ad0addf68833ef7
Hello agl@golang.org, hanwen@google.com, jpsugar@google.com, dave@cheney.net (cc: golang-dev@googlegroups.com),
Please take another look.
Message from jmpittman@google.com
2013-10-22T16:49:00+00:00jmpittmanurn:md5:1952366b6d79cc90fa80cd92b7442d8a
Ping.
Message from jpsugar@google.com
2013-10-22T17:01:45+00:00jpsugarurn:md5:afb044eb4b8e7a36b0c6b8bcd6d43bb8
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.
Message from unknown
2013-10-22T18:49:22+00:00jmpittmanurn:md5:1de1546874e8aa187604d1d511e95642
Message from jmpittman@google.com
2013-10-22T18:52:16+00:00jmpittmanurn:md5:188d32fc5964b9285390a6364a92e989
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.
Message from jpsugar@google.com
2013-10-22T18:54:31+00:00jpsugarurn:md5:9cb5817f7571f5d7d6ed383beebf7b16
Something is horribly wrong with that upload. I get "old chunk mismatch" trying to view diffs.
Message from unknown
2013-10-22T18:55:00+00:00jmpittmanurn:md5:83e1a708318ce4fd195c79d8307aef89
Message from agl@golang.org
2013-10-22T18:55:33+00:00agl1urn:md5:3364e57980bdc4718c385fbec077c5bc
Yea, the upload went wonky.
Message from jmpittman@google.com
2013-10-22T18:55:55+00:00jmpittmanurn:md5:c0bac4b8b774257d2f9c4a3d9a682201
Yeah, weird. I uploaded again. Try it now.
Message from jpsugar@google.com
2013-10-22T19:05:47+00:00jpsugarurn:md5:851060533cceb42cd40e93d5f667ce4a
LGTM
Message from agl@golang.org
2013-10-22T19:12:56+00:00agl1urn:md5:4851c66cb0dc3f31d1cf2df26f46d86a
*** 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>