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

Issue 13272055: code review 13272055: go.crypto/ssh: fix certificate parsing/marshaling. (Closed)

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

Description

go.crypto/ssh: fix certificate parsing/marshaling. The change to add the PublicKey interface accidentally caused certificate handling to expect an extra copy of the private key algorithm name in the binary representation. This change adapts a suitable parsing API and adds a test to ensure that cert handling isn't easily broken in the future.

Patch Set 1 #

Patch Set 2 : diff -r c923f02daf74 https://code.google.com/p/go.crypto #

Patch Set 3 : diff -r c923f02daf74 https://code.google.com/p/go.crypto #

Total comments: 8

Patch Set 4 : diff -r c923f02daf74 https://code.google.com/p/go.crypto #

Patch Set 5 : diff -r c923f02daf74 https://code.google.com/p/go.crypto #

Total comments: 2

Patch Set 6 : diff -r c923f02daf74 https://code.google.com/p/go.crypto #

Patch Set 7 : diff -r 2cd6b3b93cdb https://code.google.com/p/go.crypto #

Total comments: 2

Patch Set 8 : diff -r 2cd6b3b93cdb https://code.google.com/p/go.crypto #

Total comments: 4

Patch Set 9 : diff -r 2cd6b3b93cdb https://code.google.com/p/go.crypto #

Unified diffs Side-by-side diffs Delta from patch set Stats (+64 lines, -22 lines) Patch
M ssh/agent.go View 1 1 chunk +1 line, -1 line 0 comments Download
M ssh/certs.go View 1 2 3 4 5 6 7 4 chunks +17 lines, -5 lines 0 comments Download
M ssh/common.go View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M ssh/keys.go View 1 2 3 4 5 6 7 8 6 chunks +15 lines, -14 lines 0 comments Download
M ssh/keys_test.go View 1 2 3 4 5 6 2 chunks +29 lines, -0 lines 0 comments Download
M ssh/server.go View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 18
jpsugar
Hello agl@golang.org, hanwen@google.com (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go.crypto
10 years, 6 months ago (2013-09-18 19:18:30 UTC) #1
agl1
LGTM, will will wait for hanwen before landing. I'd encourage a longer change description.
10 years, 6 months ago (2013-09-18 19:27:33 UTC) #2
jpsugar
On Wed, Sep 18, 2013 at 12:27 PM, <agl@golang.org> wrote: > I'd encourage a longer ...
10 years, 6 months ago (2013-09-18 19:33:43 UTC) #3
agl1
On Wed, Sep 18, 2013 at 3:33 PM, JP Sugarbroad <jpsugar@google.com> wrote: > Sure, what ...
10 years, 6 months ago (2013-09-18 19:44:30 UTC) #4
jpsugar
On Wed, Sep 18, 2013 at 12:44 PM, Adam Langley <agl@golang.org> wrote: > I'd just ...
10 years, 6 months ago (2013-09-18 20:01:11 UTC) #5
hanwen-google
LGTM Looks good, modulo nits. Thanks for adding test coverage; that was dearly needed. https://codereview.appspot.com/13272055/diff/6001/ssh/certs.go ...
10 years, 6 months ago (2013-09-18 20:02:38 UTC) #6
jpsugar
https://codereview.appspot.com/13272055/diff/6001/ssh/certs.go File ssh/certs.go (right): https://codereview.appspot.com/13272055/diff/6001/ssh/certs.go#newcode69 ssh/certs.go:69: return "" On 2013/09/18 20:02:39, hanwen-google wrote: > return ...
10 years, 6 months ago (2013-09-18 20:10:48 UTC) #7
hanwen-google
https://codereview.appspot.com/13272055/diff/19001/ssh/certs.go File ssh/certs.go (right): https://codereview.appspot.com/13272055/diff/19001/ssh/certs.go#newcode71 ssh/certs.go:71: return "" panic here?
10 years, 6 months ago (2013-09-19 08:26:30 UTC) #8
jpsugar
https://codereview.appspot.com/13272055/diff/19001/ssh/certs.go File ssh/certs.go (right): https://codereview.appspot.com/13272055/diff/19001/ssh/certs.go#newcode71 ssh/certs.go:71: return "" On 2013/09/19 08:26:30, hanwen-google wrote: > panic ...
10 years, 6 months ago (2013-09-19 17:09:04 UTC) #9
hanwen-google
LGTM
10 years, 6 months ago (2013-09-23 17:08:07 UTC) #10
jpsugar
PTAL - rebased.
10 years, 5 months ago (2013-10-07 17:07:50 UTC) #11
hanwen-google
LGTM https://codereview.appspot.com/13272055/diff/35001/ssh/certs.go File ssh/certs.go (right): https://codereview.appspot.com/13272055/diff/35001/ssh/certs.go#newcode64 ssh/certs.go:64: // If a non-certificate algorithm is passed, returns ...
10 years, 5 months ago (2013-10-07 18:08:43 UTC) #12
jpsugar
https://codereview.appspot.com/13272055/diff/35001/ssh/certs.go File ssh/certs.go (right): https://codereview.appspot.com/13272055/diff/35001/ssh/certs.go#newcode64 ssh/certs.go:64: // If a non-certificate algorithm is passed, returns an ...
10 years, 5 months ago (2013-10-07 18:14:59 UTC) #13
jpsugar
agl - this OK?
10 years, 5 months ago (2013-10-08 22:35:08 UTC) #14
jmpittman
I think we can change these now too. https://codereview.appspot.com/13272055/diff/40001/ssh/keys.go File ssh/keys.go (right): https://codereview.appspot.com/13272055/diff/40001/ssh/keys.go#newcode53 ssh/keys.go:53: func ...
10 years, 5 months ago (2013-10-08 22:49:02 UTC) #15
jpsugar
Thanks! https://codereview.appspot.com/13272055/diff/40001/ssh/keys.go File ssh/keys.go (right): https://codereview.appspot.com/13272055/diff/40001/ssh/keys.go#newcode53 ssh/keys.go:53: func parseAuthorizedKey(in []byte) (out interface{}, comment string, ok ...
10 years, 5 months ago (2013-10-09 16:45:19 UTC) #16
agl1
*** Submitted as https://code.google.com/p/go/source/detail?r=ede3e37127cc&repo=crypto *** go.crypto/ssh: fix certificate parsing/marshaling. The change to add the PublicKey ...
10 years, 5 months ago (2013-10-09 16:56:26 UTC) #17
jpsugar
10 years, 5 months ago (2013-10-09 16:58:27 UTC) #18
*** Abandoned ***
Sign in to reply to this message.

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