code review 5650067: go.crypto/ssh: add client support for OpenSSH certificates

go.crypto/ssh: add client support for OpenSSH certificates
Refactor key parsing, marshaling, and serialization to be a bit more flexible

[jmpittman, 2012-02-12 18:08:37]

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

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

[jmpittman, 2012-02-13 04:05:28]

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

Please take another look.

[jmpittman, 2012-02-13 05:04:19]

Please be as critical as you need with this CL.  I am still trying to fully
grasp where this code is going.  Criticisms and praises welcome.

FYI,
Instead of extending the functionality of unmarshal, I wrote a temporary
solution for parsing the certificate.  Initially I was planning to support rsa
and dsa, but I removed those bits in favor of just rsa for now (hence the "_ =
algo" line).  I mainly wanted to demonstrate what is needed to get this running.

Some other parts that were added to get this working feel a bit hackish.  What
are the plans for supporting more than just RSA keys throughout?

What I was thinking I would like to see is general support for each type of
key/cert with generic functions that handle determining what to do with each
type individually.  There is some of that now (algoname, serializePublicKey,
etc), but the coverage appears spotty.  For example, there is a parseRSA, but
not a generic parseKey that strips off the algorithm type, matches that to the
parseRSA function, and lets parseRSA handle the rest of the bytes.  As of now,
parseRSA strips off the algorithm type, makes sure it is the right kind, and
then proceeds.  I feel that by the time you get to parseRSA, you should already
know you are dealing with an rsa key.

I also have an additional file (agent.go) that adds basic client side ssh agent
support to this package, but felt that should be left for another CL.  If you
want to see it (as it makes use of some of the things I have added), just let me
know where to put it.

On 2012/02/13 04:05:28, jmpittman wrote:
> Hello mailto:golang-dev@googlegroups.com (cc: mailto:agl@golang.org,
mailto:dave@cheney.net,
> mailto:golang-dev@googlegroups.com),
> 
> Please take another look.

[agl1, 2012-02-13 15:57:00]

If there is refactoring that you believe makes the code cleaner overall then I'd
encourage you to do it. Although I do wonder why you opted to skip using
marshal/unmarshal. They would need some extending, of course, but it appears
that it would save you writing another (un)marshal function for the V01 certs.

In general, do enough people still use DSA with SSH to make it worthwhile? I
feel that DSA is now, cryptographically, a patent-based anachronism.

http://codereview.appspot.com/5650067/diff/3006/ssh/openssh.go
File ssh/openssh.go (right):

http://codereview.appspot.com/5650067/diff/3006/ssh/openssh.go#newcode12
ssh/openssh.go:12: // CertType defines the entity type an OpenSSH certificate is
used to represent.
It seems that this file could have a more specific name. 'certs.go'?

http://codereview.appspot.com/5650067/diff/3006/ssh/openssh.go#newcode41
ssh/openssh.go:41: Constraints             []tuple
is this []tuple rather than map[string]string because we need to preserve order?

http://codereview.appspot.com/5650067/diff/3006/ssh/openssh.go#newcode50
ssh/openssh.go:50: algo, in, ok := parseString(in)
you can just s/algo/_/ here and remove line 54.

http://codereview.appspot.com/5650067/diff/3006/ssh/openssh.go#newcode77
ssh/openssh.go:77: case 1:
UserCert?

http://codereview.appspot.com/5650067/diff/3006/ssh/openssh.go#newcode79
ssh/openssh.go:79: case 2:
HostCert?

http://codereview.appspot.com/5650067/diff/3006/ssh/openssh.go#newcode82
ssh/openssh.go:82: panic("Unknown certificate type.")
return with ok=false?

http://codereview.appspot.com/5650067/diff/3006/ssh/openssh.go#newcode91
ssh/openssh.go:91: vp, in, ok := parseLengthPrefixedNameList(in)
s/vp/pubCert.ValidPrincipals/? (and remove the ':')

http://codereview.appspot.com/5650067/diff/3006/ssh/openssh.go#newcode109
ssh/openssh.go:109: cons, in, ok := parseTupleList(in)
same here and below.

[jmpittman, 2012-02-13 22:18:21]

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

Please take another look.

[jmpittman, 2012-02-13 22:30:42]

For now, I skipped unmarshal/unmarshal since the approach marshal is going for
leans toward specific message types.  A cert is also not a message, just another
series of bytes embedded in a message.  There is also the parseRSA function for
parsing an rsa public key.  A cert is basically a key with additional
"certificate" like data tacked onto the end.  Unless I fabricate a byte to
prepend to the byte stream of a certificate, unmarshal will expect something to
identify it, but will have nothing useful.

(FWIW, ssh agent support will be more friendly with the unmarshal/marshal
functions as they are written now.)

As for DSA, I don't know how useful or useless it might be.  I just know that it
is supposed to be available and specifically required by RFC 4253 6.6.

ssh-dss      REQUIRED    sign Raw DSS Key
ssh-rsa      RECOMMENDED sign Raw RSA Key
pgp-sign-rsa OPTIONAL    sign OpenPGP certificates (RSA key)
pgp-sign-dss OPTIONAL    sign OpenPGP certificates (DSS key)

With the latest editions of OpenSSH certificates, the entire support includes...

ssh-rsa-cert-v00@openssh.com
ssh-dss-cert-v00@openssh.com
ssh-rsa-cert-v01@openssh.com
ssh-dss-cert-v01@openssh.com
ecdsa-sha2-nistp256-cert-v01@openssh.com
ecdsa-sha2-nistp384-cert-v01@openssh.com
ecdsa-sha2-nistp521-cert-v01@openssh.com

For some of the refactoring I was considering proposing, should it be included
in this CL or wait for another?

http://codereview.appspot.com/5650067/diff/3006/ssh/openssh.go
File ssh/openssh.go (right):

http://codereview.appspot.com/5650067/diff/3006/ssh/openssh.go#newcode12
ssh/openssh.go:12: // CertType defines the entity type an OpenSSH certificate is
used to represent.
On 2012/02/13 15:57:00, agl1 wrote:
> It seems that this file could have a more specific name. 'certs.go'?

Considering that ssh allows for OpenPGP certificates, I felt it proper to define
this as OpenSSH since that was the goal of this file...  to add support for
OpenSSH certificates.  However, certs.go is probably a fine name.  Will change
this later.

Consequently, I decided to remove the "CertType" type and just go with uint32
for these values.  I am not certain that it added much value to make it a
separate type with only two simple options.

http://codereview.appspot.com/5650067/diff/3006/ssh/openssh.go#newcode41
ssh/openssh.go:41: Constraints             []tuple
On 2012/02/13 15:57:00, agl1 wrote:
> is this []tuple rather than map[string]string because we need to preserve
order?

Yes.  Is there a better/preferred way of doing this without a []tuple?

http://codereview.appspot.com/5650067/diff/3006/ssh/openssh.go#newcode50
ssh/openssh.go:50: algo, in, ok := parseString(in)
On 2012/02/13 15:57:00, agl1 wrote:
> you can just s/algo/_/ here and remove line 54.

Yeah, forgot to remove this.  It was leftover when I was originally
differentiating between rsa & dss/dsa.

http://codereview.appspot.com/5650067/diff/3006/ssh/openssh.go#newcode77
ssh/openssh.go:77: case 1:
On 2012/02/13 15:57:00, agl1 wrote:
> UserCert?

Referenced from above.  I didn't like this little bit of code.  Changed.  PTAL.

http://codereview.appspot.com/5650067/diff/3006/ssh/openssh.go#newcode79
ssh/openssh.go:79: case 2:
On 2012/02/13 15:57:00, agl1 wrote:
> HostCert?

same

http://codereview.appspot.com/5650067/diff/3006/ssh/openssh.go#newcode82
ssh/openssh.go:82: panic("Unknown certificate type.")
On 2012/02/13 15:57:00, agl1 wrote:
> return with ok=false?

removed

http://codereview.appspot.com/5650067/diff/3006/ssh/openssh.go#newcode91
ssh/openssh.go:91: vp, in, ok := parseLengthPrefixedNameList(in)
On 2012/02/13 15:57:00, agl1 wrote:
> s/vp/pubCert.ValidPrincipals/? (and remove the ':')

Done.

http://codereview.appspot.com/5650067/diff/3006/ssh/openssh.go#newcode109
ssh/openssh.go:109: cons, in, ok := parseTupleList(in)
On 2012/02/13 15:57:00, agl1 wrote:
> same here and below.

Done.

[jmpittman, 2012-02-13 22:32:54]

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

Please take another look.

[jmpittman, 2012-02-13 22:43:19]

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

Please take another look.

[jmpittman, 2012-02-13 22:43:55]

On 2012/02/13 22:43:19, jmpittman wrote:
> Hello mailto:agl@golang.org, mailto:dave@cheney.net (cc:
mailto:golang-dev@googlegroups.com),
> 
> Please take another look.

Sorry, forgot to remove some references to the recently removed CertType.

[djm, 2012-02-14 01:51:22]

A couple of initial comments before I dive in deeper.

http://codereview.appspot.com/5650067/diff/11002/ssh/common.go
File ssh/common.go (right):

http://codereview.appspot.com/5650067/diff/11002/ssh/common.go#newcode150
ssh/common.go:150: case "ssh-dss-cert-v00@openssh.com",
"ssh-dss-cert-v01@openssh.com":
I wouldn't bother implementing the -v00 cert types, they are scheduled for
deprecation (and indeed should have been deleted in OpenSSH by now)

http://codereview.appspot.com/5650067/diff/11002/ssh/openssh.go
File ssh/openssh.go (right):

http://codereview.appspot.com/5650067/diff/11002/ssh/openssh.go#newcode58
ssh/openssh.go:58: return nil, false
I don't think there is any hard requirement that RSA e is < 2^64. I've never
seen a key use modulus > 65537 in the wild, but I don't think they are illegal.

http://codereview.appspot.com/5650067/diff/11002/ssh/openssh.go#newcode211
ssh/openssh.go:211: func parseTupleList(in []byte) (out []tuple, rest []byte, ok
bool) {
FWIW I consider the extensions and critical options more like a map than a
tuple. OpenSSH considers it an error for most name keys to appear more than once
(e.g it will refuse a cert if source-address appears multiply, but will accept
multiple "permit-user-rc").

I'll update PROTOCOL.certkeys to mention that names should be unique in these
fields.

[jmpittman, 2012-02-14 04:01:42]

http://codereview.appspot.com/5650067/diff/11002/ssh/common.go
File ssh/common.go (right):

http://codereview.appspot.com/5650067/diff/11002/ssh/common.go#newcode150
ssh/common.go:150: case "ssh-dss-cert-v00@openssh.com",
"ssh-dss-cert-v01@openssh.com":
On 2012/02/14 01:51:22, djm wrote:
> I wouldn't bother implementing the -v00 cert types, they are scheduled for
> deprecation (and indeed should have been deleted in OpenSSH by now)

Noted.  Any plans for a v02 in the near future, or will v01 be around for a bit?
 Is there potential harm in supporting v00 (i.e. security flaws)?  What are the
chances v00 will still be in use in the world for a while after deprecation?

http://codereview.appspot.com/5650067/diff/11002/ssh/openssh.go
File ssh/openssh.go (right):

http://codereview.appspot.com/5650067/diff/11002/ssh/openssh.go#newcode58
ssh/openssh.go:58: return nil, false
On 2012/02/14 01:51:22, djm wrote:
> I don't think there is any hard requirement that RSA e is < 2^64. I've never
> seen a key use modulus > 65537 in the wild, but I don't think they are
illegal.

Interesting point.  The rsa.PublicKey implementation in Go
(http://weekly.golang.org/pkg/crypto/rsa/#PublicKey) defines E (public exponent)
as an int while N (modulus) is defined as a *big.Int.  This is the primary
reason for the Int64 reference.  If this were to be changed, it would initially
need to be changed in the rsa package to define E as a big.Int.

http://codereview.appspot.com/5650067/diff/11002/ssh/openssh.go#newcode211
ssh/openssh.go:211: func parseTupleList(in []byte) (out []tuple, rest []byte, ok
bool) {
On 2012/02/14 01:51:22, djm wrote:
> FWIW I consider the extensions and critical options more like a map than a
> tuple. OpenSSH considers it an error for most name keys to appear more than
once
> (e.g it will refuse a cert if source-address appears multiply, but will accept
> multiple "permit-user-rc").
> 
> I'll update PROTOCOL.certkeys to mention that names should be unique in these
> fields.

Since the list of options appears small and finite, at one time I had thought to
hard code them in as variables.  However, that doesn't do much for
marshal/unmarshaling.  It would be more useful for construction and possibly
some level of validation.

Do you suggest producing an error during parsing if a name is found more than
once?  Or only do that for critical options, but not extensions?  Should the
validation/refusal be reserved for the server side or also be considered for the
client side?

Another question popped up about the use of a []tuple vs map[string]string.  I
had gone with tuple since I thought the order mattered.  Does the ordering
matter within critical options or within extensions?  If a cert is unmarshaled
and marshaled will a change in order to these options cause any problems?

[agl1, 2012-02-17 17:22:40]

On Mon, Feb 13, 2012 at 11:01 PM,  <jmpittman@google.com> wrote:
> Noted.  Any plans for a v02 in the near future, or will v01 be around
> for a bit?  Is there potential harm in supporting v00 (i.e. security
> flaws)?  What are the chances v00 will still be in use in the world for
> a while after deprecation?

(p.s. I support dropping the -v00 code unless there's a clear use.
Less code is good.)

> Interesting point.  The rsa.PublicKey implementation in Go
> (http://weekly.golang.org/pkg/crypto/rsa/#PublicKey) defines E (public
> exponent) as an int while N (modulus) is defined as a *big.Int.  This is
> the primary reason for the Int64 reference.  If this were to be changed,
> it would initially need to be changed in the rsa package to define E as
> a big.Int.

Large public modulus is `legal', but a complete waste of CPU time. I'm
happy to not support them. In [1] they found two large exponents out
of 11.5M RSA keys and they suspect that they were bugs.

[1] http://eprint.iacr.org/2012/064.pdf



Cheers

AGL

[jmpittman, 2012-02-19 03:36:26]

Hello agl@golang.org, dave@cheney.net, djm@djm.net.au (cc:
golang-dev@googlegroups.com),

Please take another look.

[jmpittman, 2012-02-19 03:46:26]

On 2012/02/19 03:36:26, jmpittman wrote:
> Hello mailto:agl@golang.org, mailto:dave@cheney.net, mailto:djm@djm.net.au
(cc:
> mailto:golang-dev@googlegroups.com),
> 
> Please take another look.

I added some of the refactoring I was considering.  For the moment, I want to
leave in the v00 support.  v00 is currently the only type I have available to
use.  Although, I am being led to believe that v01 certs are on my horizon
within a few months.  At that point, I won't have a stake in using v00 any
longer.

I suppose, for the purpose of this implementation, I could remove them and just
keep a patch around for myself to play with as needed.  If no one is in favor of
keeping them around (and it sounds like that is the case), I'll do that on my
next update to this CL after some more review.

[dfc, 2012-02-19 07:02:31]

Thank you for your continued work on this addition. My only other comment would
be to suggest making the "ssh-rsa-cert-v00@openssh.com" and friends constants.

http://codereview.appspot.com/5650067/diff/13002/ssh/certs.go
File ssh/certs.go (right):

http://codereview.appspot.com/5650067/diff/13002/ssh/certs.go#newcode18
ssh/certs.go:18: )
Do these need to be typed constants? If these are constants defined in the
OpenSSH cert format maybe a comment or a link to the spec would be useful.
Otherwise maybe


const (
        UserCert = iota
        HostCert 
)

http://codereview.appspot.com/5650067/diff/13002/ssh/certs.go#newcode348
ssh/certs.go:348: length := 4 /* uint32 length prefix */
// uint32 length prefix

http://codereview.appspot.com/5650067/diff/13002/ssh/certs.go#newcode393
ssh/certs.go:393: length := 4 /* uint32 length prefix */
// uint32 length prefix

http://codereview.appspot.com/5650067/diff/13002/ssh/certs.go#newcode408
ssh/certs.go:408: func parseSignature(in []byte) (out signature, rest []byte, ok
bool) {
I think the use of named return values, and using return out, nil, false is a
bit confusing. I would suggest either dropping them, and declaring 

var out signature

at the top of the function, or making the function look more like parseTupleList
above.

[jmpittman, 2012-02-20 05:13:14]

Hello agl@golang.org, dave@cheney.net, djm@djm.net.au (cc:
golang-dev@googlegroups.com),

Please take another look.

[jmpittman, 2012-02-20 05:14:12]

http://codereview.appspot.com/5650067/diff/13002/ssh/certs.go
File ssh/certs.go (right):

http://codereview.appspot.com/5650067/diff/13002/ssh/certs.go#newcode18
ssh/certs.go:18: )
On 2012/02/19 07:02:31, dfc wrote:
> Do these need to be typed constants? If these are constants defined in the
> OpenSSH cert format maybe a comment or a link to the spec would be useful.
> Otherwise maybe
> 
> 
> const (
>         UserCert = iota
>         HostCert 
> )

I am not certain they need to be typed here as a uint32.  When they are used in
a certificate, used for comparison, or encoded for going over the wire, they
need to be treated as a uint32.

Reworded for clarity and linked to the spec.

http://codereview.appspot.com/5650067/diff/13002/ssh/certs.go#newcode348
ssh/certs.go:348: length := 4 /* uint32 length prefix */
On 2012/02/19 07:02:31, dfc wrote:
> // uint32 length prefix

Done.

http://codereview.appspot.com/5650067/diff/13002/ssh/certs.go#newcode393
ssh/certs.go:393: length := 4 /* uint32 length prefix */
On 2012/02/19 07:02:31, dfc wrote:
> // uint32 length prefix

Done.

http://codereview.appspot.com/5650067/diff/13002/ssh/certs.go#newcode408
ssh/certs.go:408: func parseSignature(in []byte) (out signature, rest []byte, ok
bool) {
On 2012/02/19 07:02:31, dfc wrote:
> I think the use of named return values, and using return out, nil, false is a
> bit confusing. I would suggest either dropping them, and declaring 
> 
> var out signature
> 
> at the top of the function, or making the function look more like
parseTupleList
> above.

PTAL

[djm, 2012-02-21 00:08:58]

On Tue, Feb 14, 2012 at 3:01 PM,  <jmpittman@google.com> wrote:
>
> http://codereview.appspot.com/5650067/diff/11002/ssh/common.go
> File ssh/common.go (right):
>
> http://codereview.appspot.com/5650067/diff/11002/ssh/common.go#newcode150
> ssh/common.go:150: case "ssh-dss-cert-v00@openssh.com",
> "ssh-dss-cert-v01@openssh.com":
> On 2012/02/14 01:51:22, djm wrote:
>>
>> I wouldn't bother implementing the -v00 cert types, they are scheduled
>
> for
>>
>> deprecation (and indeed should have been deleted in OpenSSH by now)
>
>
> Noted.  Any plans for a v02 in the near future, or will v01 be around
> for a bit?

I'd expect v01 to be around for a while; between critical options,
extensions and the reserved field there is plenty of scope for adding
missing bits without needing a new format.

> Is there potential harm in supporting v00 (i.e. security
> flaws)?

Increased attack surface is the main risk. Supporting both
simultaneously in OpenSSH already caused one bug.

> What are the chances v00 will still be in use in the world for
> a while after deprecation?

There might be one or two users, but OpenSSH has generated v01 certs
by default for >18 months.

>> I'll update PROTOCOL.certkeys to mention that names should be unique
>
> in these
>>
>> fields.
>
>
> Since the list of options appears small and finite, at one time I had
> thought to hard code them in as variables.  However, that doesn't do
> much for marshal/unmarshaling.  It would be more useful for construction
> and possibly some level of validation.
>
> Do you suggest producing an error during parsing if a name is found more
> than once?  Or only do that for critical options, but not extensions?
> Should the validation/refusal be reserved for the server side or also be
> considered for the client side?

You should definitely ban duplication of critical options, as these
are supposed to be restrictive and any ambiguity could lead to
unintended access (consider conflicting "source-address"
restrictions).

For extensions, you could probably afford to be more liberal in
interpretation without nasty consequence. I think OpenSSH effectively
treats the options here as idempotent.

There are no client-side options at the moment, but IMO the same rules
should apply.

> Another question popped up about the use of a []tuple vs
> map[string]string.  I had gone with tuple since I thought the order
> mattered.  Does the ordering matter within critical options or within
> extensions?  If a cert is unmarshaled and marshaled will a change in
> order to these options cause any problems?

The PROTOCOL.certkeys defines them as to occur in lexical order, but
OpenSSH doesn't enforce this at the moment. I'd recommend that you
output them in order when you marshal, but I don't see much
consequence to being a bit liberal when unmarshalling.

-d

[jmpittman, 2012-02-21 04:07:36]

> I'd expect v01 to be around for a while; between critical options,
> extensions and the reserved field there is plenty of scope for adding
> missing bits without needing a new format.

Good to know.

> Increased attack surface is the main risk. Supporting both
> simultaneously in OpenSSH already caused one bug.

Fair enough.

> There might be one or two users, but OpenSSH has generated v01 certs
> by default for >18 months.

I will go ahead and remove them from this CL, but keep a patch around for myself
for the time being until I am switched over to v01 in my regular use.  If there
are any reasons to add them back it will not take much.

> You should definitely ban duplication of critical options, as these
> are supposed to be restrictive and any ambiguity could lead to
> unintended access (consider conflicting "source-address"
> restrictions).
> 
> For extensions, you could probably afford to be more liberal in
> interpretation without nasty consequence. I think OpenSSH effectively
> treats the options here as idempotent.
> 
> There are no client-side options at the moment, but IMO the same rules
> should apply.

> The PROTOCOL.certkeys defines them as to occur in lexical order, but
> OpenSSH doesn't enforce this at the moment. I'd recommend that you
> output them in order when you marshal, but I don't see much
> consequence to being a bit liberal when unmarshalling.
> 
> -d

With respect to duplicate critical options, do you have a recommendation about
whether to fail the deserialization process or to perform a separate validation
after parsing is complete?  My first thought is that deserialization should be
concerned only with validity of structure, not validity of options.  If it does
not affect the client side at the moment, it sounds as though the server side
should do separate validation.  Thoughts?

Switching from a tuple to a map[string]string might simplify some things. 
However, a map would not allow for being liberal with multiple extensions using
the same name.

The concern I had with using a map and handling ordering had to do with any
comparison, validation, or hashing that might occur on the certificate.  For
example, say a certificate is encountered with the options not in lexical order.
 If I go the route of unmarshaling as options are encountered, but then I
marshal in lexical order, will that cause any issues?  The original and new
serialized forms as well as base64 encoded forms would not be the same.  If
there are multiple extensions using the same name, they would be deduped by the
map.  Upon re-serialization, the duplicates would be missing and the certificate
length would be shorter.

If none of that actually matters in spec, in theory, or in practice, then
switching to a map is relatively simple.

[jmpittman, 2012-02-21 18:53:22]

Hello agl@golang.org, dave@cheney.net, djm@djm.net.au (cc:
golang-dev@googlegroups.com),

Please take another look.

[agl1, 2012-02-21 23:08:02]

http://codereview.appspot.com/5650067/diff/21006/ssh/certs.go
File ssh/certs.go (right):

http://codereview.appspot.com/5650067/diff/21006/ssh/certs.go#newcode46
ssh/certs.go:46: Key                     interface{} // rsa or dsa PublicKey
insert '*' before 'PublicKey'

http://codereview.appspot.com/5650067/diff/21006/ssh/certs.go#newcode63
ssh/certs.go:63: return nil, nil, false
All the return statements in this function (except the last) can simply be
"return" if you use another name for |out|

http://codereview.appspot.com/5650067/diff/21006/ssh/certs.go#newcode72
ssh/certs.go:72: out.Key = *rsaPubKey
I don't think you want to dereference here (or below for |dsaPubKey|)

http://codereview.appspot.com/5650067/diff/21006/ssh/certs.go#newcode144
ssh/certs.go:144: case rsa.PublicKey:
*rsa.PublicKey (and on the other lines here)

http://codereview.appspot.com/5650067/diff/21006/ssh/certs.go#newcode149
ssh/certs.go:149: pubKey = marshalPubDSA(&k)
default:
  panic("ssh: unknown public key type in cert")

http://codereview.appspot.com/5650067/diff/21006/ssh/certs.go#newcode182
ssh/certs.go:182: marshalSignature(r, cert.Signature)
r = marshalSignature(r, cert.Signature)
if len(r) > 0 {
  panic("internal error")
}

http://codereview.appspot.com/5650067/diff/21006/ssh/certs.go#newcode189
ssh/certs.go:189: length += stringLength([]byte(name))
length += 4 /* length prefix */
length += len(name)

(otherwise I fear lots of copying will happen.)

http://codereview.appspot.com/5650067/diff/21006/ssh/certs.go#newcode196
ssh/certs.go:196: nlBytes := to[:length]
rather than introducing n1Bytes, can't you just say:
to = marshalUint32(to, uint32(length-4)))

and so on, finally returning to?

http://codereview.appspot.com/5650067/diff/21006/ssh/certs.go#newcode228
ssh/certs.go:228: length += stringLength([]byte(t.Name))
I would inline stringLength again here.

http://codereview.appspot.com/5650067/diff/21006/ssh/certs.go#newcode236
ssh/certs.go:236: tlBytes := to[:length]
ditto about using 'to'.

http://codereview.appspot.com/5650067/diff/21006/ssh/keys.go
File ssh/keys.go (right):

http://codereview.appspot.com/5650067/diff/21006/ssh/keys.go#newcode23
ssh/keys.go:23: return *key, rest, ok
don't dereference key (here and elsewhere).

http://codereview.appspot.com/5650067/diff/21006/ssh/keys.go#newcode29
ssh/keys.go:29: return *key, rest, ok
default:
  panic(...)

[jmpittman, 2012-02-22 18:26:45]

Hello agl@golang.org, dave@cheney.net, djm@djm.net.au (cc:
golang-dev@googlegroups.com),

Please take another look.

[jmpittman, 2012-02-22 18:28:00]

Thanks! Had a few questions.  Comments in common.go.

http://codereview.appspot.com/5650067/diff/21006/ssh/certs.go
File ssh/certs.go (right):

http://codereview.appspot.com/5650067/diff/21006/ssh/certs.go#newcode46
ssh/certs.go:46: Key                     interface{} // rsa or dsa PublicKey
On 2012/02/21 23:08:02, agl1 wrote:
> insert '*' before 'PublicKey'

Done.

http://codereview.appspot.com/5650067/diff/21006/ssh/certs.go#newcode63
ssh/certs.go:63: return nil, nil, false
On 2012/02/21 23:08:02, agl1 wrote:
> All the return statements in this function (except the last) can simply be
> "return" if you use another name for |out|

Done.

http://codereview.appspot.com/5650067/diff/21006/ssh/certs.go#newcode72
ssh/certs.go:72: out.Key = *rsaPubKey
On 2012/02/21 23:08:02, agl1 wrote:
> I don't think you want to dereference here (or below for |dsaPubKey|)

Done.

http://codereview.appspot.com/5650067/diff/21006/ssh/certs.go#newcode144
ssh/certs.go:144: case rsa.PublicKey:
On 2012/02/21 23:08:02, agl1 wrote:
> *rsa.PublicKey (and on the other lines here)

Done.

http://codereview.appspot.com/5650067/diff/21006/ssh/certs.go#newcode149
ssh/certs.go:149: pubKey = marshalPubDSA(&k)
On 2012/02/21 23:08:02, agl1 wrote:
> default:
>   panic("ssh: unknown public key type in cert")

Done.

http://codereview.appspot.com/5650067/diff/21006/ssh/certs.go#newcode182
ssh/certs.go:182: marshalSignature(r, cert.Signature)
On 2012/02/21 23:08:02, agl1 wrote:
> r = marshalSignature(r, cert.Signature)
> if len(r) > 0 {
>   panic("internal error")
> }

Done.

http://codereview.appspot.com/5650067/diff/21006/ssh/certs.go#newcode189
ssh/certs.go:189: length += stringLength([]byte(name))
On 2012/02/21 23:08:02, agl1 wrote:
> length += 4 /* length prefix */
> length += len(name)
> 
> (otherwise I fear lots of copying will happen.)

Done.

http://codereview.appspot.com/5650067/diff/21006/ssh/certs.go#newcode196
ssh/certs.go:196: nlBytes := to[:length]
On 2012/02/21 23:08:02, agl1 wrote:
> rather than introducing n1Bytes, can't you just say:
> to = marshalUint32(to, uint32(length-4)))
> 
> and so on, finally returning to?

Done.

http://codereview.appspot.com/5650067/diff/21006/ssh/certs.go#newcode228
ssh/certs.go:228: length += stringLength([]byte(t.Name))
On 2012/02/21 23:08:02, agl1 wrote:
> I would inline stringLength again here.

Done.

http://codereview.appspot.com/5650067/diff/21006/ssh/certs.go#newcode236
ssh/certs.go:236: tlBytes := to[:length]
On 2012/02/21 23:08:02, agl1 wrote:
> ditto about using 'to'.

Done.

http://codereview.appspot.com/5650067/diff/21006/ssh/common.go
File ssh/common.go (right):

http://codereview.appspot.com/5650067/diff/21006/ssh/common.go#newcode172
ssh/common.go:172: case rsa.PublicKey:
Since the parse and marshal functions as well as structs for keys and certs are
passing around pointers, should this be case *rsa.PublicKey (and so on...)? 
This is the reason why I was dereferencing in those other places.  A
*rsa.PublicKey was causing a panic since it expects an rsa.PublicKey.

http://codereview.appspot.com/5650067/diff/21006/ssh/common.go#newcode192
ssh/common.go:192: case rsa.PublicKey:
Ditto from 172.

http://codereview.appspot.com/5650067/diff/21006/ssh/keys.go
File ssh/keys.go (right):

http://codereview.appspot.com/5650067/diff/21006/ssh/keys.go#newcode23
ssh/keys.go:23: return *key, rest, ok
On 2012/02/21 23:08:02, agl1 wrote:
> don't dereference key (here and elsewhere).

Done.

http://codereview.appspot.com/5650067/diff/21006/ssh/keys.go#newcode29
ssh/keys.go:29: return *key, rest, ok
On 2012/02/21 23:08:02, agl1 wrote:
> default:
>   panic(...)

Done.

[agl1, 2012-02-23 15:43:30]

I've done http://codereview.appspot.com/5686067/ to address the *rsa.PublicKey
stuff. If you can respin the patch in light of that and confirm that everything
is working then I think this is good to land.

http://codereview.appspot.com/5650067/diff/21007/ssh/certs.go
File ssh/certs.go (right):

http://codereview.appspot.com/5650067/diff/21007/ssh/certs.go#newcode185
ssh/certs.go:185: marshalSignature(r, cert.Signature)
there's no "r = " on this line so I think the next line will always panic. (See
comment on this line in the previous patch set.)

[jmpittman, 2012-02-23 16:08:20]

Hello agl@golang.org, dave@cheney.net, djm@djm.net.au (cc:
golang-dev@googlegroups.com),

Please take another look.

[jmpittman, 2012-02-23 16:10:22]

http://codereview.appspot.com/5650067/diff/21007/ssh/certs.go
File ssh/certs.go (right):

http://codereview.appspot.com/5650067/diff/21007/ssh/certs.go#newcode185
ssh/certs.go:185: marshalSignature(r, cert.Signature)
On 2012/02/23 15:43:30, agl1 wrote:
> there's no "r = " on this line so I think the next line will always panic.
(See
> comment on this line in the previous patch set.)

Done.

[agl1, 2012-02-24 17:52:12]

*** Submitted as
http://code.google.com/p/go/source/detail?r=82579bac06b4&repo=crypto ***

go.crypto/ssh: add client support for OpenSSH certificates
Refactor key parsing, marshaling, and serialization to be a bit more flexible

R=agl, dave, djm
CC=golang-dev
http://codereview.appspot.com/5650067

Committer: Adam Langley <agl@golang.org>