code review 6855107: ssh: add functions for public keys in wire & auth keys ...

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.

[sledbetter, 2012-11-27 18:48:09]

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

[sledbetter, 2012-11-27 18:49:17]

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.

[agl1, 2012-11-27 21:10:45]

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.

[sledbetter, 2012-11-28 02:09:02]

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.

[sledbetter, 2012-11-28 02:10:18]

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

Please take another look.

[agl1, 2012-11-29 21:15:22]

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.

[sledbetter, 2012-11-30 02:41:03]

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.

[agl1, 2012-11-30 16:21:05]

Forgot to hg upload?

[sledbetter, 2012-11-30 17:54:17]

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

Please take another look.

[agl1, 2012-11-30 22:36:45]

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.)

[sledbetter, 2012-12-04 18:14:54]

Pulled your changes in, caught one change needed in parser for backslashed
quotes, added more test cases.

PTAL.

[agl1, 2012-12-10 16:11:52]

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

[bradfitz, 2012-12-10 16:27:20]

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/contr...
>
>
https://codereview.appspot.**com/6855107/<https://codereview.appspot.com/6855...
>

[agl1, 2012-12-10 17:21:17]

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

[agl1, 2012-12-10 22:43:26]

*** 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>