|
|
Created:
12 years, 2 months ago by jmpittman Modified:
12 years, 1 month ago Reviewers:
CC:
agl1, dfc, djm, golang-dev Visibility:
Public. |
Descriptiongo.crypto/ssh: add client support for OpenSSH certificates
Refactor key parsing, marshaling, and serialization to be a bit more flexible
Patch Set 1 #Patch Set 2 : diff -r d25f08a48e35 https://code.google.com/p/go.crypto #Patch Set 3 : diff -r d25f08a48e35 https://code.google.com/p/go.crypto #Patch Set 4 : diff -r d25f08a48e35 https://code.google.com/p/go.crypto #
Total comments: 16
Patch Set 5 : diff -r d25f08a48e35 https://code.google.com/p/go.crypto #Patch Set 6 : diff -r d25f08a48e35 https://code.google.com/p/go.crypto #Patch Set 7 : diff -r d25f08a48e35 https://code.google.com/p/go.crypto #
Total comments: 6
Patch Set 8 : diff -r 89f10772a982 https://code.google.com/p/go.crypto #
Total comments: 8
Patch Set 9 : diff -r 89f10772a982 https://code.google.com/p/go.crypto #Patch Set 10 : diff -r 080d73e214b6 https://code.google.com/p/go.crypto #
Total comments: 26
Patch Set 11 : diff -r 080d73e214b6 https://code.google.com/p/go.crypto #
Total comments: 2
Patch Set 12 : diff -r 53e036e53a3c https://code.google.com/p/go.crypto #
MessagesTotal messages: 27
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
Sign in to reply to this message.
Hello golang-dev@googlegroups.com (cc: agl@golang.org, dave@cheney.net, golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
Hello agl@golang.org, dave@cheney.net (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
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.
Sign in to reply to this message.
Hello agl@golang.org, dave@cheney.net (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
Hello agl@golang.org, dave@cheney.net (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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?
Sign in to reply to this message.
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
Sign in to reply to this message.
Hello agl@golang.org, dave@cheney.net, djm@djm.net.au (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
Hello agl@golang.org, dave@cheney.net, djm@djm.net.au (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
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
Sign in to reply to this message.
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
Sign in to reply to this message.
> 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.
Sign in to reply to this message.
Hello agl@golang.org, dave@cheney.net, djm@djm.net.au (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
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(...)
Sign in to reply to this message.
Hello agl@golang.org, dave@cheney.net, djm@djm.net.au (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.)
Sign in to reply to this message.
Hello agl@golang.org, dave@cheney.net, djm@djm.net.au (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
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.
Sign in to reply to this message.
*** 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>
Sign in to reply to this message.
|