|
|
Created:
11 years, 2 months ago by jmpittman Modified:
7 months, 2 weeks ago CC:
agl1, hanwen-google, jpsugar, dave_cheney.net, golang-dev Visibility:
Public. |
Descriptiongo.crypto/ssh: Add certificate verification, step up support for authorized keys
Patch Set 1 #Patch Set 2 : diff -r 812c06b5a384 https://code.google.com/p/go.crypto #Patch Set 3 : diff -r 812c06b5a384 https://code.google.com/p/go.crypto #Patch Set 4 : diff -r 812c06b5a384 https://code.google.com/p/go.crypto #Patch Set 5 : diff -r 812c06b5a384 https://code.google.com/p/go.crypto #
Total comments: 9
Patch Set 6 : diff -r 812c06b5a384 https://code.google.com/p/go.crypto #Patch Set 7 : diff -r 7fb39a59524c https://code.google.com/p/go.crypto #Patch Set 8 : diff -r 7fb39a59524c https://code.google.com/p/go.crypto #Patch Set 9 : diff -r 7fb39a59524c https://code.google.com/p/go.crypto #Patch Set 10 : diff -r 7fb39a59524c https://code.google.com/p/go.crypto #Patch Set 11 : diff -r 7fb39a59524c https://code.google.com/p/go.crypto #
Total comments: 8
Patch Set 12 : diff -r bb19605bfacc https://code.google.com/p/go.crypto #Patch Set 13 : diff -r bb19605bfacc https://code.google.com/p/go.crypto #Patch Set 14 : diff -r bb19605bfacc https://code.google.com/p/go.crypto #
Total comments: 1
Patch Set 15 : diff -r 04f39b6a609b https://code.google.com/p/go.crypto #Patch Set 16 : diff -r 04f39b6a609b https://code.google.com/p/go.crypto #
Total comments: 2
Patch Set 17 : diff -r 04f39b6a609b https://code.google.com/p/go.crypto #
Total comments: 14
Patch Set 18 : diff -r 5ff5636e18c9 https://code.google.com/p/go.crypto #Patch Set 19 : diff -r 5ff5636e18c9 https://code.google.com/p/go.crypto #
Total comments: 4
Patch Set 20 : diff -r 5ff5636e18c9 https://code.google.com/p/go.crypto #Patch Set 21 : diff -r 5ff5636e18c9 https://code.google.com/p/go.crypto #
MessagesTotal messages: 50
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
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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. :)
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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
Sign in to reply to this message.
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.
Sign in to reply to this message.
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
Sign in to reply to this message.
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
Sign in to reply to this message.
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.
Sign in to reply to this message.
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?
Sign in to reply to this message.
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.
Sign in to reply to this message.
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?
Sign in to reply to this message.
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
Sign in to reply to this message.
Hello agl@golang.org, hanwen@google.com, jpsugar@google.com (cc: dave@cheney.net, golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
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?
Sign in to reply to this message.
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.
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
Can I get dfc or agl to take a look at this?
Sign in to reply to this message.
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
Sign in to reply to this message.
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.
Sign in to reply to this message.
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
Sign in to reply to this message.
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.
Sign in to reply to this message.
Hello agl@golang.org, hanwen@google.com, jpsugar@google.com, dave@cheney.net (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
Ping.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
Something is horribly wrong with that upload. I get "old chunk mismatch" trying to view diffs.
Sign in to reply to this message.
Yea, the upload went wonky.
Sign in to reply to this message.
Yeah, weird. I uploaded again. Try it now.
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
*** 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>
Sign in to reply to this message.
Message was sent while issue was closed.
Ok
Sign in to reply to this message.
Message was sent while issue was closed.
Ok
Sign in to reply to this message.
Message was sent while issue was closed.
Ok
Sign in to reply to this message.
Message was sent while issue was closed.
On 2024/04/23 00:16:58, SAmiq wrote: > <font style="vertical-align: inherit;"><font style="vertical-align: inherit;">نعم</font></font>
Sign in to reply to this message.
Message was sent while issue was closed.
Ok
Sign in to reply to this message.
Message was sent while issue was closed.
Ok
Sign in to reply to this message.
Message was sent while issue was closed.
Ok
Sign in to reply to this message.
Message was sent while issue was closed.
Ok
Sign in to reply to this message.
Message was sent while issue was closed.
On 2024/04/23 00:21:54, SAmiq wrote: > Ok
Sign in to reply to this message.
Message was sent while issue was closed.
On 2024/04/23 00:21:54, SAmiq wrote: > Ok
Sign in to reply to this message.
Message was sent while issue was closed.
On 2013/10/10 18:32:57, 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 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.
Sign in to reply to this message.
Message was sent while issue was closed.
Sign in to reply to this message.
|