Code review - Issue 6855107: code review 6855107: ssh: add functions for public keys in wire & auth keys ...https://codereview.appspot.com/2012-12-10T22:43:26+00:00rietveld
Message from unknown
2012-11-27T18:41:41+00:00sledbetterurn:md5:d544402ba44f1358b11e8c33e8a21099
Message from unknown
2012-11-27T18:41:45+00:00sledbetterurn:md5:cfb2336eae5956c64b407e5a9715e6b6
Message from unknown
2012-11-27T18:48:06+00:00sledbetterurn:md5:fd706b35bcf314d5e2675df8fe0ef665
Message from sledbetter@google.com
2012-11-27T18:48:09+00:00sledbetterurn:md5:6298a1b20ea73b51a406e4cc793d8142
Hello golang-dev@googlegroups.com (cc: agl@chromium.org, dayveday@gmail.com),
I'd like you to review this change to
https://code.google.com/p/go.crypto
Message from unknown
2012-11-27T18:49:11+00:00sledbetterurn:md5:9c73bf878498296c1566ef06c5374781
Message from sledbetter@google.com
2012-11-27T18:49:17+00:00sledbetterurn:md5:97aa3e52cd8e549ba2c292f419ffaa9e
Hello agl@golang.org, dave@cheney.net, golang-dev@googlegroups.com (cc: agl@chromium.org, dayveday@gmail.com, golang-dev@googlegroups.com),
Please take another look.
Message from agl@golang.org
2012-11-27T21:10:45+00:00agl1urn:md5:94c91a8bd56b3b136febece589b6645c
https://codereview.appspot.com/6855107/diff/6001/ssh/keys.go
File ssh/keys.go (right):
https://codereview.appspot.com/6855107/diff/6001/ssh/keys.go#newcode125
ssh/keys.go:125: // authorized_keys2 files for OpenSSH
authorized_keys2 has been replaced by plain authorized_keys these days I thought.
https://codereview.appspot.com/6855107/diff/6001/ssh/keys.go#newcode136
ssh/keys.go:136: out, rest, ok = parsePubKey(key)
A `rest' output would typically receive the remainder of the input after processing. But here it receives the remainder of the base64 data.
I would expect this function to find the first '\n' in |in| and assign rest to everything after that. Then remove any trailing '\r'. Then attempt to process a key. Possibly it should skip lines until it finds a valid key.
https://codereview.appspot.com/6855107/diff/6001/ssh/keys.go#newcode155
ssh/keys.go:155:
remove extra blank line.
https://codereview.appspot.com/6855107/diff/6001/ssh/keys.go#newcode164
ssh/keys.go:164: b64Key := base64.StdEncoding.EncodeToString(keyData)
If you're going to use a bytes.Buffer, it would seem better to do:
encoder := base64.NewEncoder(base64.StdEncoding, b)
encoder.Write(serializePublickey(key))
encoder.Close()
https://codereview.appspot.com/6855107/diff/6001/ssh/keys.go#newcode165
ssh/keys.go:165: b.WriteString(" ")
b.WriteByte(' ')
https://codereview.appspot.com/6855107/diff/6001/ssh/keys.go#newcode167
ssh/keys.go:167: return b.Bytes()
b.WriteByte('\n')
https://codereview.appspot.com/6855107/diff/6001/ssh/test/keys_test.go
File ssh/test/keys_test.go (right):
https://codereview.appspot.com/6855107/diff/6001/ssh/test/keys_test.go#newcode14
ssh/test/keys_test.go:14:
remove blank line.
Message from sledbetter@google.com
2012-11-28T02:09:02+00:00sledbetterurn:md5:f3a9c5d4734afe65d9b271f97a2b0fbe
https://codereview.appspot.com/6855107/diff/6001/ssh/keys.go
File ssh/keys.go (right):
https://codereview.appspot.com/6855107/diff/6001/ssh/keys.go#newcode125
ssh/keys.go:125: // authorized_keys2 files for OpenSSH
On 2012/11/27 21:10:45, agl1 wrote:
> authorized_keys2 has been replaced by plain authorized_keys these days I
> thought.
you're right; removed.
https://codereview.appspot.com/6855107/diff/6001/ssh/keys.go#newcode136
ssh/keys.go:136: out, rest, ok = parsePubKey(key)
On 2012/11/27 21:10:45, agl1 wrote:
> A `rest' output would typically receive the remainder of the input after
> processing. But here it receives the remainder of the base64 data.
>
> I would expect this function to find the first '\n' in |in| and assign rest to
> everything after that. Then remove any trailing '\r'. Then attempt to process a
> key. Possibly it should skip lines until it finds a valid key.
At the last minute I changed this to capture the comment, but didn't add a test. I guess I have an issue now. Originally I used serializePublickey to do comparisons in ServerConfig's PublickeyCallback member; but its useful also are serializing/parsing authorized_keys files, so I wrote these functions to perform that, unfortunately now closing off the functionality I was originally after. I've rewritten these functions so I can do that while still exporting the original functions I need.
https://codereview.appspot.com/6855107/diff/6001/ssh/keys.go#newcode155
ssh/keys.go:155:
On 2012/11/27 21:10:45, agl1 wrote:
> remove extra blank line.
Done.
https://codereview.appspot.com/6855107/diff/6001/ssh/keys.go#newcode164
ssh/keys.go:164: b64Key := base64.StdEncoding.EncodeToString(keyData)
On 2012/11/27 21:10:45, agl1 wrote:
> If you're going to use a bytes.Buffer, it would seem better to do:
>
> encoder := base64.NewEncoder(base64.StdEncoding, b)
> encoder.Write(serializePublickey(key))
> encoder.Close()
Done.
https://codereview.appspot.com/6855107/diff/6001/ssh/keys.go#newcode165
ssh/keys.go:165: b.WriteString(" ")
On 2012/11/27 21:10:45, agl1 wrote:
> b.WriteByte(' ')
Done.
https://codereview.appspot.com/6855107/diff/6001/ssh/keys.go#newcode167
ssh/keys.go:167: return b.Bytes()
On 2012/11/27 21:10:45, agl1 wrote:
> b.WriteByte('\n')
Done.
https://codereview.appspot.com/6855107/diff/6001/ssh/test/keys_test.go
File ssh/test/keys_test.go (right):
https://codereview.appspot.com/6855107/diff/6001/ssh/test/keys_test.go#newcode14
ssh/test/keys_test.go:14:
On 2012/11/27 21:10:45, agl1 wrote:
> remove blank line.
Done.
Message from unknown
2012-11-28T02:10:16+00:00sledbetterurn:md5:d94a334c732a32abcedbd26845e767d1
Message from sledbetter@google.com
2012-11-28T02:10:18+00:00sledbetterurn:md5:b96a1cfe290221f8ee294f0559706084
Hello agl@golang.org, dave@cheney.net, golang-dev@googlegroups.com (cc: agl@chromium.org, golang-dev@googlegroups.com),
Please take another look.
Message from agl@golang.org
2012-11-29T21:15:22+00:00agl1urn:md5:45b19ede115c3b0d4bb880dba4eb3d89
https://codereview.appspot.com/6855107/diff/4004/ssh/keys.go
File ssh/keys.go (right):
https://codereview.appspot.com/6855107/diff/4004/ssh/keys.go#newcode124
ssh/keys.go:124: // ParseAuthorizedKeys parses a protocol 2 entry from an
s/protocol 2 entry/public key/
https://codereview.appspot.com/6855107/diff/4004/ssh/keys.go#newcode125
ssh/keys.go:125: // authorized_keys file used in OpenSSH
Should reference "sshd(8) manual page"
https://codereview.appspot.com/6855107/diff/4004/ssh/keys.go#newcode126
ssh/keys.go:126: func ParseAuthorizedKeys(in []byte) (out interface{}, comment []byte, options [][]byte, rest []byte, ok bool) {
The function only parses a single key so probably shouldn't have a plural name.
https://codereview.appspot.com/6855107/diff/4004/ssh/keys.go#newcode130
ssh/keys.go:130: return nil, nil, nil, nil, false
can just be "return"
https://codereview.appspot.com/6855107/diff/4004/ssh/keys.go#newcode136
ssh/keys.go:136: rest = bytes.TrimSpace(in[end:])
that's an odd `rest' return value because it won't include any trailing space and it might start with a '\n'.
https://codereview.appspot.com/6855107/diff/4004/ssh/keys.go#newcode141
ssh/keys.go:141: goto startOfEntry
looks like there's a for loop in here.
https://codereview.appspot.com/6855107/diff/4004/ssh/keys.go#newcode152
ssh/keys.go:152: case hostAlgoRSA, hostAlgoDSA:
This list is already incomplete in more recent OpenSSH versions. It looks like a better way to figure out whether fields[0] is a key type or options is the presence of an '='.
https://codereview.appspot.com/6855107/diff/4004/ssh/keys.go#newcode178
ssh/keys.go:178: func MarshalAuthorizedKeys(key interface{}) []byte {
likewise about the plural function name.
Message from sledbetter@google.com
2012-11-30T02:41:03+00:00sledbetterurn:md5:e0ef3e773323b8f1e9b3dace4b3bf4ab
I cleaned up the whole CRLF parsing and added to and cleaned up the tests.
https://codereview.appspot.com/6855107/diff/4004/ssh/keys.go
File ssh/keys.go (right):
https://codereview.appspot.com/6855107/diff/4004/ssh/keys.go#newcode124
ssh/keys.go:124: // ParseAuthorizedKeys parses a protocol 2 entry from an
On 2012/11/29 21:15:22, agl1 wrote:
> s/protocol 2 entry/public key/
I said protocol 2 because I'm explicitly ignoring protocol 1 keys here. Do I need to actually account for them and process them here, or are the deprecated enough to not worry about?
https://codereview.appspot.com/6855107/diff/4004/ssh/keys.go#newcode125
ssh/keys.go:125: // authorized_keys file used in OpenSSH
On 2012/11/29 21:15:22, agl1 wrote:
> Should reference "sshd(8) manual page"
Done.
https://codereview.appspot.com/6855107/diff/4004/ssh/keys.go#newcode126
ssh/keys.go:126: func ParseAuthorizedKeys(in []byte) (out interface{}, comment []byte, options [][]byte, rest []byte, ok bool) {
On 2012/11/29 21:15:22, agl1 wrote:
> The function only parses a single key so probably shouldn't have a plural name.
Done.
https://codereview.appspot.com/6855107/diff/4004/ssh/keys.go#newcode152
ssh/keys.go:152: case hostAlgoRSA, hostAlgoDSA:
On 2012/11/29 21:15:22, agl1 wrote:
> This list is already incomplete in more recent OpenSSH versions. It looks like a
> better way to figure out whether fields[0] is a key type or options is the
> presence of an '='.
There are a couple non-'=' options (cert-authority, no-pty), so it can't be that. The man page mentions that options never start with a number and to use that, but none of the keytypes start with a number either, so I'm not sure how to deal with it from that approach.
I've added the other ECDSA keytypes but panic (or should they return !ok ?) for now.
https://codereview.appspot.com/6855107/diff/4004/ssh/keys.go#newcode178
ssh/keys.go:178: func MarshalAuthorizedKeys(key interface{}) []byte {
On 2012/11/29 21:15:22, agl1 wrote:
> likewise about the plural function name.
Done.
Message from agl@golang.org
2012-11-30T16:21:05+00:00agl1urn:md5:484a00f8b5692860c333ace83a82b39f
Forgot to hg upload?
Message from unknown
2012-11-30T17:54:13+00:00sledbetterurn:md5:945d7aaefd4b585e3226f391c70e6092
Message from sledbetter@google.com
2012-11-30T17:54:17+00:00sledbetterurn:md5:01f0db28f6e9ec644f1eb578324a404f
Hello agl@golang.org, dave@cheney.net, golang-dev@googlegroups.com (cc: agl@chromium.org, golang-dev@googlegroups.com),
Please take another look.
Message from agl@golang.org
2012-11-30T22:36:45+00:00agl1urn:md5:ac8f3ae5a4410a0606e197989fba6f27
https://codereview.appspot.com/6855107/diff/3003/ssh/keys.go
File ssh/keys.go (right):
https://codereview.appspot.com/6855107/diff/3003/ssh/keys.go#newcode16
ssh/keys.go:16: // Keytypes supported by OpenSSH 1.5.9
1.5.9? Maybe 5.9?
https://codereview.appspot.com/6855107/diff/3003/ssh/keys.go#newcode152
ssh/keys.go:152: if len(in) != 0 && !bytes.HasPrefix(in, []byte("#")) {
replace
!bytes.HasPrefix(in, []byte("#"))
with
in[0] != '#'
https://codereview.appspot.com/6855107/diff/3003/ssh/keys.go#newcode172
ssh/keys.go:172: panic("unexpected key type")
I think a panic is too much.
Ideally we would continue to the next line, which suggests that all this should be in the for loop.
https://codereview.appspot.com/6855107/diff/3003/ssh/keys.go#newcode174
ssh/keys.go:174: options = strings.Split(string(fields[0]), ",")
What if the options contain a space in a quoted value?
I'm afraid that I think this parser needs to be significantly more complex in order to have similar behaviour to the parser in OpenSSH's auth2-pubkey.c.
Please see https://codereview.appspot.com/6855115
That would be my suggestion. I'd strongly suggesting copying the keys_test.go file over after reviewing it as it contains cases that I think this code will fail on.
You may wish to start iterating on the keys.go, in which case it would be good to copy it over, do an hg upload and then start changing things. (So that the inter-patchset diffs work in the codereview tool.)
Message from unknown
2012-12-03T23:24:25+00:00sledbetterurn:md5:907e9d3f6ebc5c7c7491556dd49ed38e
Message from unknown
2012-12-04T18:13:08+00:00sledbetterurn:md5:391555870baca890efda0976fd828ba1
Message from sledbetter@google.com
2012-12-04T18:14:54+00:00sledbetterurn:md5:740e1c7dd96d0a50e7973615294949d7
Pulled your changes in, caught one change needed in parser for backslashed quotes, added more test cases.
PTAL.
Message from agl@golang.org
2012-12-10T16:11:52+00:00agl1urn:md5:18f84075dba113e28180766adbd00642
Sorry, this got washed away by other stuff last week. There's a couple of things I might fix up when landing, but I think this is basically good to go.
Have you signed the ICLA? http://golang.org/doc/contribute.html#copyright
Message from bradfitz@golang.org
2012-12-10T16:27:20+00:00bradfitzurn:md5:fb7cc409cfc060401da228f94442899e
He's @google.com, so no CLA needed.
On Mon, Dec 10, 2012 at 11:11 AM, <agl@golang.org> wrote:
> Sorry, this got washed away by other stuff last week. There's a couple
> of things I might fix up when landing, but I think this is basically
> good to go.
>
> Have you signed the ICLA?
> http://golang.org/doc/**contribute.html#copyright<http://golang.org/doc/contribute.html#copyright>
>
> https://codereview.appspot.**com/6855107/<https://codereview.appspot.com/6855107/>
>
Message from agl@golang.org
2012-12-10T17:21:17+00:00agl1urn:md5:df11bbf400bb2da111517b9896b34ef5
On Mon, Dec 10, 2012 at 11:21 AM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> He's @google.com, so no CLA needed.
Oh, good point. Sending out CONTRIBUTORS change now.
Cheers
AGL
Message from agl@golang.org
2012-12-10T22:43:26+00:00agl1urn:md5:1a37bfb612b4dbbdf57f8c3735741b12
*** Submitted as https://code.google.com/p/go/source/detail?r=159cc2447982&repo=crypto ***
ssh: add functions for public keys in wire & auth keys format.
This allows easy import/export of public keys in the format
expected by OpenSSH for authorized_keys files, as well as
allowing comparisons with ServerConfig's PublickeyCallback.
Fixes issue 3908.
R=agl, dave, golang-dev, bradfitz
CC=agl, golang-dev
https://codereview.appspot.com/6855107
Committer: Adam Langley <agl@golang.org>