Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(122)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 years, 1 month ago by jmpittman
Modified:
6 years, 1 month ago
Reviewers:
jpsugar, dfc, hanwen-google
CC:
agl1, hanwen-google, jpsugar, dfc, golang-dev
Visibility:
Public.

Description

go.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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+187 lines, -103 lines) Patch
M ssh/certs.go View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 5 chunks +73 lines, -42 lines 0 comments Download
A ssh/certs_test.go View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +55 lines, -0 lines 0 comments Download
M ssh/keys.go View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +5 lines, -24 lines 0 comments Download
M ssh/keys_test.go View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 4 chunks +54 lines, -37 lines 0 comments Download

Messages

Total messages: 37
jmpittman
Hello agl1 (cc: dfc, golang-dev@googlegroups.com, hanwen-google, jpsugar), I'd like you to review this change to ...
6 years, 1 month ago (2013-10-10 03:17:04 UTC) #1
hanwen-google
This CL has both functionality and cosmetics. Can you put the cosmetic changes into a ...
6 years, 1 month ago (2013-10-10 12:34:24 UTC) #2
jmpittman
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 ...
6 years, 1 month ago (2013-10-10 16:40:00 UTC) #3
hanwen-google
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: ...
6 years, 1 month ago (2013-10-10 16:56:21 UTC) #4
jmpittman
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: ...
6 years, 1 month ago (2013-10-10 18:32:57 UTC) #5
jpsugar
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: ...
6 years, 1 month ago (2013-10-10 23:08:17 UTC) #6
jmpittman
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: ...
6 years, 1 month ago (2013-10-11 16:13:42 UTC) #7
jmpittman
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 > ...
6 years, 1 month ago (2013-10-11 20:30:42 UTC) #8
jmpittman
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 ...
6 years, 1 month ago (2013-10-11 20:50:41 UTC) #9
hanwen-google
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 ...
6 years, 1 month ago (2013-10-12 09:53:58 UTC) #10
hanwen-google
On Fri, Oct 11, 2013 at 10:30 PM, <jmpittman@google.com> wrote: > I have the tests ...
6 years, 1 month ago (2013-10-12 09:54:13 UTC) #11
jmpittman
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 ...
6 years, 1 month ago (2013-10-14 16:44:15 UTC) #12
jmpittman
On 2013/10/12 09:54:13, hanwen-google wrote: > On Fri, Oct 11, 2013 at 10:30 PM, <mailto:jmpittman@google.com> ...
6 years, 1 month ago (2013-10-14 16:52:29 UTC) #13
hanwen-google
On Mon, Oct 14, 2013 at 9:52 AM, <jmpittman@google.com> wrote: > > > Agreed. I ...
6 years, 1 month ago (2013-10-15 17:27:42 UTC) #14
jmpittman
On 2013/10/15 17:27:42, hanwen-google wrote: > On Mon, Oct 14, 2013 at 9:52 AM, <mailto:jmpittman@google.com> ...
6 years, 1 month ago (2013-10-15 17:35:42 UTC) #15
hanwen-google
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 ...
6 years, 1 month ago (2013-10-15 17:52:55 UTC) #16
jmpittman
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 > ...
6 years, 1 month ago (2013-10-15 18:57:30 UTC) #17
jmpittman
On second thought, I would like to remove the test for parseOpenSSHCert as you suggested ...
6 years, 1 month ago (2013-10-16 20:39:14 UTC) #18
hanwen-google
On 2013/10/16 20:39:14, jmpittman wrote: > On second thought, I would like to remove the ...
6 years, 1 month ago (2013-10-16 20:40:23 UTC) #19
jmpittman
Hello agl@golang.org, hanwen@google.com, jpsugar@google.com (cc: dave@cheney.net, golang-dev@googlegroups.com), Please take another look.
6 years, 1 month ago (2013-10-16 20:43:40 UTC) #20
hanwen-google
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 ...
6 years, 1 month ago (2013-10-16 20:45:23 UTC) #21
jmpittman
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 ...
6 years, 1 month ago (2013-10-16 20:51:07 UTC) #22
hanwen-google
LGTM
6 years, 1 month ago (2013-10-17 00:48:55 UTC) #23
jmpittman
Can I get dfc or agl to take a look at this?
6 years, 1 month ago (2013-10-17 22:46:34 UTC) #24
dfc
Thanks. Code LGTM. I've added a bunch of unhelpful nits to the review. I really ...
6 years, 1 month ago (2013-10-19 11:24:23 UTC) #25
jmpittman
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 ...
6 years, 1 month ago (2013-10-20 05:17:21 UTC) #26
hanwen-google
On Sat, Oct 19, 2013 at 10:17 PM, <jmpittman@google.com> wrote: > This code is mostly ...
6 years, 1 month ago (2013-10-20 05:23:21 UTC) #27
jmpittman
Yeah, I was considering some kind of "core" marshaling method that would add the necessary ...
6 years, 1 month ago (2013-10-20 05:29:22 UTC) #28
jmpittman
Hello agl@golang.org, hanwen@google.com, jpsugar@google.com, dave@cheney.net (cc: golang-dev@googlegroups.com), Please take another look.
6 years, 1 month ago (2013-10-20 05:45:40 UTC) #29
jmpittman
Ping.
6 years, 1 month ago (2013-10-22 16:49:00 UTC) #30
jpsugar
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 ...
6 years, 1 month ago (2013-10-22 17:01:45 UTC) #31
jmpittman
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 ...
6 years, 1 month ago (2013-10-22 18:52:16 UTC) #32
jpsugar
Something is horribly wrong with that upload. I get "old chunk mismatch" trying to view ...
6 years, 1 month ago (2013-10-22 18:54:31 UTC) #33
agl1
Yea, the upload went wonky.
6 years, 1 month ago (2013-10-22 18:55:33 UTC) #34
jmpittman
Yeah, weird. I uploaded again. Try it now.
6 years, 1 month ago (2013-10-22 18:55:55 UTC) #35
jpsugar
LGTM
6 years, 1 month ago (2013-10-22 19:05:47 UTC) #36
agl1
6 years, 1 month ago (2013-10-22 19:12:56 UTC) #37
*** 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.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b