Merge lp:~axwalk/juju-core/ssh-fingerprint-platform-independent into lp:~go-bot/juju-core/trunk

[Andrew Wilkins]

utils/ssh: platform-independent KeyFingerprint

Windows CLI is broken as keyFingerprint has
not been implemented. We can't rely on
ssh-keygen there.

I've rewritten the keyFingerprint function in
pure Go. This involves writing a parser for
authorized_keys lines. The base64-decoded key
is MD5-summed, and then formatted with colons
separating the hex-encoded bytes.

https://codereview.appspot.com/49710043/

utils/ssh: platform-independent KeyFingerprint

Windows CLI is broken as keyFingerprint has
not been implemented. We can't rely on
ssh-keygen there.

I've rewritten the keyFingerprint function in
pure Go. This involves writing a parser for
authorized_keys lines. The base64-decoded key
is MD5-summed, and then formatted with colons
separating the hex-encoded bytes.

https://codereview.appspot.com/49710043/

[Andrew Wilkins (axwalk)]

Reviewers: mp+200980_code.launchpad.net,

Message:
Please take a look.

Description:
utils/ssh: platform-independent KeyFingerprint

Windows CLI is broken as keyFingerprint has
not been implemented. We can't rely on
ssh-keygen there.

I've rewritten the keyFingerprint function in
pure Go. This involves writing a parser for
authorized_keys lines. The base64-decoded key
is MD5-summed, and then formatted with colons
separating the hex-encoded bytes.

https://code.launchpad.net/~axwalk/juju-core/ssh-fingerprint-platform-independent/+merge/200980

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/49710043/

Affected files (+186, -45 lines):
   A [revision details]
   M utils/ssh/authorisedkeys.go
   M utils/ssh/authorisedkeys_test.go
   M utils/ssh/fingerprint.go
   D utils/ssh/fingerprint_win.go

[Tim Penhey (thumper)]

https://codereview.appspot.com/49710043/diff/1/utils/ssh/fingerprint.go
File utils/ssh/fingerprint.go (right):

https://codereview.appspot.com/49710043/diff/1/utils/ssh/fingerprint.go#newcode4
utils/ssh/fingerprint.go:4: // +build !windows
Probably want to remove this line, yes?

https://codereview.appspot.com/49710043/diff/1/utils/ssh/fingerprint.go#newcode15
utils/ssh/fingerprint.go:15: // in authorized_key format.
Is there an RFC to reference about the format of the fingerprint?

https://codereview.appspot.com/49710043/diff/1/utils/ssh/fingerprint.go#newcode16
utils/ssh/fingerprint.go:16: func KeyFingerprint(key string)
(fingerprint, comment string, err error) {
Can we add a few tests?

https://codereview.appspot.com/49710043/

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/49710043/diff/1/utils/ssh/fingerprint.go
File utils/ssh/fingerprint.go (right):

https://codereview.appspot.com/49710043/diff/1/utils/ssh/fingerprint.go#newcode4
utils/ssh/fingerprint.go:4: // +build !windows
On 2014/01/10 02:05:36, thumper wrote:
> Probably want to remove this line, yes?

Heh, yes, thanks.

https://codereview.appspot.com/49710043/diff/1/utils/ssh/fingerprint.go#newcode15
utils/ssh/fingerprint.go:15: // in authorized_key format.
On 2014/01/10 02:05:36, thumper wrote:
> Is there an RFC to reference about the format of the fingerprint?

Apparently so: updated with reference to RFC4716.

https://codereview.appspot.com/49710043/diff/1/utils/ssh/fingerprint.go#newcode16
utils/ssh/fingerprint.go:16: func KeyFingerprint(key string)
(fingerprint, comment string, err error) {
On 2014/01/10 02:05:36, thumper wrote:
> Can we add a few tests?

I have added a direct test, though for the record, it's at 100% coverage
with the current (indirect) tests.

https://codereview.appspot.com/49710043/

[Ian Booth (wallyworld)]

LGTM

https://codereview.appspot.com/49710043/

[Roger Peppe (rogpeppe)]

Looks great in general; one thought below.

https://codereview.appspot.com/49710043/diff/20001/utils/ssh/authorisedkeys.go
File utils/ssh/authorisedkeys.go (right):

https://codereview.appspot.com/49710043/diff/20001/utils/ssh/authorisedkeys.go#newcode75
utils/ssh/authorisedkeys.go:75: if strings.HasPrefix(lower, o) {
I'm not sure that we need to know about all the
valid options and key types here. I think it
would be more robust (and future-proof) if we
could avoid that.

How about something like this:

Rename skipOptions to parseField, and make it return
the field instead of skipping it.

In ParseAuthorizedKeys, call parseField.
If it returns a field starting with a digit,
you know you've got a version 1 line and
can error out or parse the rest of the fields as appropriate.

Otherwise it's either the options or a key type.
If the options contains no comma or equals sign,
there's no fully unambiguous way of determining
which without knowing all the possible options
or key types, but I think it would be reasonable
to get the next field (which is either a key type
or a public key) and see if it's a public key.
Any public key is base64 and going to be reasonably big,
(if length > 30 || has any char other than [\-a-z0-9])
should be a reasonable diagnosis.

That way we can hopefully continue working without
change when ssh adds more options and/or key types.

https://codereview.appspot.com/49710043/

[Andrew Wilkins (axwalk)]

On 2014/01/10 09:31:19, rog wrote:
> Looks great in general; one thought below.

https://codereview.appspot.com/49710043/diff/20001/utils/ssh/authorisedkeys.go
> File utils/ssh/authorisedkeys.go (right):

https://codereview.appspot.com/49710043/diff/20001/utils/ssh/authorisedkeys.go#newcode75
> utils/ssh/authorisedkeys.go:75: if strings.HasPrefix(lower, o) {
> I'm not sure that we need to know about all the
> valid options and key types here. I think it
> would be more robust (and future-proof) if we
> could avoid that.

> How about something like this:

> Rename skipOptions to parseField, and make it return
> the field instead of skipping it.

> In ParseAuthorizedKeys, call parseField.
> If it returns a field starting with a digit,
> you know you've got a version 1 line and
> can error out or parse the rest of the fields as appropriate.

> Otherwise it's either the options or a key type.
> If the options contains no comma or equals sign,
> there's no fully unambiguous way of determining
> which without knowing all the possible options
> or key types, but I think it would be reasonable
> to get the next field (which is either a key type
> or a public key) and see if it's a public key.
> Any public key is base64 and going to be reasonably big,
> (if length > 30 || has any char other than [\-a-z0-9])
> should be a reasonable diagnosis.

> That way we can hopefully continue working without
> change when ssh adds more options and/or key types.

I did wonder about this, and you've swayed me. I really wasn't keen on
detecting the key based on something ambiguous, but I think you're right
and it's good enough. I will update the code when I finish off what I'm
currently doing.

https://codereview.appspot.com/49710043/

[Andrew Wilkins (axwalk)]

On 2014/01/10 10:10:38, hanwenn wrote:
> drive-by comment: have you considered adding some of this to gosshnew?
There is
> already some incomplete authorizedkey parsing in the SSH library.

Hi there!

I did think that it would be useful in a public library, but was going
for expediency here (we're using go.crypto/ssh at the moment; can't
really rely on gosshnew until it's nailed down). I can submit a patch to
gosshnew when my code is polished, and then we'll use it in the future
when your refactoring is done.

https://codereview.appspot.com/49710043/

=== modified file 'utils/ssh/authorisedkeys.go'
--- utils/ssh/authorisedkeys.go	2013-12-23 05:18:58 +0000
+++ utils/ssh/authorisedkeys.go	2014-01-10 02:44:47 +0000
@@ -4,6 +4,7 @@
 package ssh
 
 import (
+	"encoding/base64"
 	"fmt"
 	"io/ioutil"
 	"os"
@@ -33,9 +34,112 @@
 	authKeysFile = "authorized_keys"
 )
 
-// KeyFingerprint returns the fingerprint and comment for the specified key, using
-// the OS dependent function keyFingerprint.
-var KeyFingerprint = keyFingerprint
+// case-insensive public authorized_keys options
+var validOptions = [...]string{
+	"cert-authority",
+	"command",
+	"environment",
+	"from",
+	"no-agent",
+	"no-port-forwarding",
+	"no-pty",
+	"no-user",
+	"no-X11-forwarding",
+	"permitopen",
+	"principals",
+	"tunnel",
+}
+
+var validKeytypes = [...]string{
+	"ecdsa-sha2-nistp256",
+	"ecdsa-sha2-nistp384",
+	"ecdsa-sha2-nistp521",
+	"ssh-dss",
+	"ssh-rsa",
+}
+
+type AuthorisedKey struct {
+	KeyType string
+	Key     []byte
+	Comment string
+}
+
+// skipOptions takes a non-comment line from an
+// authorized_keys file, and returns the remainder
+// of the line after skipping any options at the
+// beginning of the line.
+func skipOptions(line string) string {
+	found := false
+	lower := strings.ToLower(line)
+	for _, o := range validOptions {
+		if strings.HasPrefix(lower, o) {
+			line = line[len(o):]
+			found = true
+			break
+		}
+	}
+	if !found {
+		return line
+	}
+	// Skip to the next unquoted whitespace, returning the remainder.
+	// Double quotes may be escaped with \".
+	var quoted bool
+	for i := 0; i < len(line); i++ {
+		switch line[i] {
+		case ' ', '\t':
+			if !quoted {
+				return strings.TrimLeft(line[i+1:], " \t")
+			}
+		case '\\':
+			if i+1 < len(line) && line[i+1] == '"' {
+				i++
+			}
+		case '"':
+			quoted = !quoted
+		}
+	}
+	return ""
+}
+
+// ParseAuthorisedKey parses a non-comment line from an
+// authorized_keys file and returns the constituent parts.
+// Based on description in "man sshd".
+//
+// TODO(axw) support version 1 format?
+func ParseAuthorisedKey(line string) (*AuthorisedKey, error) {
+	withoutOptions := skipOptions(line)
+	var keytype, key, comment string
+	if i := strings.IndexAny(withoutOptions, " \t"); i == -1 {
+		// There must be at least two fields: keytype and key.
+		return nil, fmt.Errorf("malformed line: %q", line)
+	} else {
+		keytype = withoutOptions[:i]
+		key = strings.TrimSpace(withoutOptions[i+1:])
+	}
+	validKeytype := false
+	for _, kt := range validKeytypes {
+		if keytype == kt {
+			validKeytype = true
+			break
+		}
+	}
+	if !validKeytype {
+		return nil, fmt.Errorf("invalid keytype %q in line %q", keytype, line)
+	}
+	// Split key/comment (if any)
+	if i := strings.IndexAny(key, " \t"); i != -1 {
+		key, comment = key[:i], key[i+1:]
+	}
+	keyBytes, err := base64.StdEncoding.DecodeString(key)
+	if err != nil {
+		return nil, err
+	}
+	return &AuthorisedKey{
+		KeyType: keytype,
+		Key:     keyBytes,
+		Comment: comment,
+	}, nil
+}
 
 // SplitAuthorisedKeys extracts a key slice from the specified key data,
 // by splitting the key data into lines and ignoring comments and blank lines.
@@ -157,7 +261,7 @@
 		return err
 	}
 	for _, newKey := range newKeys {
-		fingerprint, comment, err := keyFingerprint(newKey)
+		fingerprint, comment, err := KeyFingerprint(newKey)
 		if err != nil {
 			return err
 		}
@@ -165,7 +269,7 @@
 			return fmt.Errorf("cannot add ssh key without comment")
 		}
 		for _, key := range existingKeys {
-			existingFingerprint, existingComment, err := keyFingerprint(key)
+			existingFingerprint, existingComment, err := KeyFingerprint(key)
 			if err != nil {
 				// Only log a warning if the unrecognised key line is not a comment.
 				if key[0] != '#' {
@@ -202,7 +306,7 @@
 	var sshKeys = make(map[string]string)
 	var keyComments = make(map[string]string)
 	for _, key := range existingKeyData {
-		fingerprint, comment, err := keyFingerprint(key)
+		fingerprint, comment, err := KeyFingerprint(key)
 		if err != nil {
 			logger.Debugf("keeping unrecognised existing ssh key %q: %v", key, err)
 			keysToWrite = append(keysToWrite, key)
@@ -248,13 +352,13 @@
 	}
 	var existingNonKeyLines []string
 	for _, line := range existingKeyData {
-		_, _, err := keyFingerprint(line)
+		_, _, err := KeyFingerprint(line)
 		if err != nil {
 			existingNonKeyLines = append(existingNonKeyLines, line)
 		}
 	}
 	for _, newKey := range newKeys {
-		_, comment, err := keyFingerprint(newKey)
+		_, comment, err := KeyFingerprint(newKey)
 		if err != nil {
 			return err
 		}
@@ -275,7 +379,7 @@
 	}
 	var keys []string
 	for _, key := range keyData {
-		fingerprint, comment, err := keyFingerprint(key)
+		fingerprint, comment, err := KeyFingerprint(key)
 		if err != nil {
 			// Only log a warning if the unrecognised key line is not a comment.
 			if key[0] != '#' {
@@ -302,19 +406,19 @@
 const JujuCommentPrefix = "Juju:"
 
 func EnsureJujuComment(key string) string {
-	_, comment, err := keyFingerprint(key)
+	ak, err := ParseAuthorisedKey(key)
 	// Just return an invalid key as is.
 	if err != nil {
 		logger.Warningf("invalid Juju ssh key %s: %v", key, err)
 		return key
 	}
-	if comment == "" {
+	if ak.Comment == "" {
 		return key + " " + JujuCommentPrefix + "sshkey"
 	} else {
 		// Add the Juju prefix to the comment if necessary.
-		if !strings.HasPrefix(comment, JujuCommentPrefix) {
-			commentIndex := strings.LastIndex(key, comment)
-			return key[:commentIndex] + JujuCommentPrefix + comment
+		if !strings.HasPrefix(ak.Comment, JujuCommentPrefix) {
+			commentIndex := strings.LastIndex(key, ak.Comment)
+			return key[:commentIndex] + JujuCommentPrefix + ak.Comment
 		}
 	}
 	return key

=== modified file 'utils/ssh/authorisedkeys_test.go'
--- utils/ssh/authorisedkeys_test.go	2013-12-23 05:18:58 +0000
+++ utils/ssh/authorisedkeys_test.go	2014-01-10 02:44:47 +0000
@@ -4,6 +4,7 @@
 package ssh_test
 
 import (
+	"encoding/base64"
 	stdtesting "testing"
 
 	gc "launchpad.net/gocheck"
@@ -234,3 +235,53 @@
 		c.Assert(actual, gc.DeepEquals, test.expected)
 	}
 }
+
+func b64(s string) string {
+	return base64.StdEncoding.EncodeToString([]byte(s))
+}
+
+func (s *AuthorisedKeysKeysSuite) TestParseAuthorisedKey(c *gc.C) {
+	for i, test := range []struct {
+		line    string
+		keytype string
+		key     string
+		comment string
+		err     string
+	}{{
+		line:    "ssh-rsa " + b64("abc def"),
+		keytype: "ssh-rsa",
+		key:     "abc def",
+	}, {
+		line:    "ssh-dss " + b64("abc def"),
+		keytype: "ssh-dss",
+		key:     "abc def",
+	}, {
+		line:    "ssh-rsa " + b64("abc def") + " a b c",
+		keytype: "ssh-rsa",
+		key:     "abc def",
+		comment: "a b c",
+	}, {
+		line: "ssh-xsa blah",
+		err:  "invalid keytype \"ssh-xsa\" in line \"ssh-xsa blah\"",
+	}, {
+		// options should be skipped
+		line:    `no-pty,principals="\"",command="\!" ssh-rsa ` + b64("blah"),
+		keytype: "ssh-rsa",
+		key:     "blah",
+	}, {
+		line: "ssh-rsa",
+		err:  "malformed line: \"ssh-rsa\"",
+	}} {
+		c.Logf("test %d: %s", i, test.line)
+		ak, err := ssh.ParseAuthorisedKey(test.line)
+		if test.err != "" {
+			c.Assert(err, gc.ErrorMatches, test.err)
+		} else {
+			c.Assert(err, gc.IsNil)
+			c.Assert(ak, gc.Not(gc.IsNil))
+			c.Assert(ak.KeyType, gc.Equals, test.keytype)
+			c.Assert(string(ak.Key), gc.Equals, test.key)
+			c.Assert(ak.Comment, gc.Equals, test.comment)
+		}
+	}
+}

=== renamed file 'utils/ssh/fingerprint_notwin.go' => 'utils/ssh/fingerprint.go'
--- utils/ssh/fingerprint_notwin.go	2013-12-09 03:09:14 +0000
+++ utils/ssh/fingerprint.go	2014-01-10 02:44:47 +0000
@@ -1,33 +1,31 @@
 // Copyright 2013 Canonical Ltd.
 // Licensed under the AGPLv3, see LICENCE file for details.
-//
-// +build !windows
 
 package ssh
 
 import (
+	"bytes"
+	"crypto/md5"
 	"fmt"
-	"launchpad.net/juju-core/utils"
 )
 
-// keyFingerprint returns the fingerprint and comment for the specified key.
-// It calls ssh-keygen to do the work as there is no equivalent Go implementation.
-func keyFingerprint(key string) (fingerprint, comment string, err error) {
-	// Instead of invoking ssh-keygen directly, it has to be called indirectly via a
-	// shell command because it doesn't read from stdin properly.
-	// See https://bugzilla.mindrot.org/show_bug.cgi?id=1477
-	shellCmd := fmt.Sprintf("ssh-keygen -lf /dev/stdin <<<'%s'", key)
-	output, err := utils.RunCommand("bash", "-c", shellCmd)
+// KeyFingerprint returns the fingerprint and comment for the specified key
+// in authorized_key format. Fingerprints are generated according to RFC4716.
+// See ttp://www.ietf.org/rfc/rfc4716.txt, section 4.
+func KeyFingerprint(key string) (fingerprint, comment string, err error) {
+	ak, err := ParseAuthorisedKey(key)
 	if err != nil {
 		return "", "", fmt.Errorf("generating key fingerprint: %v", err)
 	}
-	var ignore string
-	n, err := fmt.Sscanf(output, "%s %s %s", &ignore, &fingerprint, &comment)
-	if n < 3 {
-		return "", "", fmt.Errorf("unexpected ssh-keygen output %q: %v", output, err)
-	}
-	if comment == "/dev/stdin" {
-		comment = ""
-	}
-	return fingerprint, comment, nil
+	hash := md5.New()
+	hash.Write(ak.Key)
+	sum := hash.Sum(nil)
+	var buf bytes.Buffer
+	for i := 0; i < hash.Size(); i++ {
+		if i > 0 {
+			buf.WriteByte(':')
+		}
+		buf.WriteString(fmt.Sprintf("%02x", sum[i]))
+	}
+	return buf.String(), ak.Comment, nil
 }

=== added file 'utils/ssh/fingerprint_test.go'
--- utils/ssh/fingerprint_test.go	1970-01-01 00:00:00 +0000
+++ utils/ssh/fingerprint_test.go	2014-01-10 02:44:47 +0000
@@ -0,0 +1,36 @@
+// Copyright 2014 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package ssh_test
+
+import (
+	gc "launchpad.net/gocheck"
+
+	"launchpad.net/juju-core/testing/testbase"
+	"launchpad.net/juju-core/utils/ssh"
+	sshtesting "launchpad.net/juju-core/utils/ssh/testing"
+)
+
+type FingerprintSuite struct {
+	testbase.LoggingSuite
+}
+
+var _ = gc.Suite(&FingerprintSuite{})
+
+func (s *FingerprintSuite) TestKeyFingerprint(c *gc.C) {
+	keys := []sshtesting.SSHKey{
+		sshtesting.ValidKeyOne,
+		sshtesting.ValidKeyTwo,
+		sshtesting.ValidKeyThree,
+	}
+	for _, k := range keys {
+		fingerprint, _, err := ssh.KeyFingerprint(k.Key)
+		c.Assert(err, gc.IsNil)
+		c.Assert(fingerprint, gc.Equals, k.Fingerprint)
+	}
+}
+
+func (s *FingerprintSuite) TestKeyFingerprintError(c *gc.C) {
+	_, _, err := ssh.KeyFingerprint("invalid key")
+	c.Assert(err, gc.ErrorMatches, `generating key fingerprint: invalid keytype "invalid" in line "invalid key"`)
+}

=== removed file 'utils/ssh/fingerprint_win.go'
--- utils/ssh/fingerprint_win.go	2013-12-10 01:32:02 +0000
+++ utils/ssh/fingerprint_win.go	1970-01-01 00:00:00 +0000
@@ -1,13 +0,0 @@
-// Copyright 2013 Canonical Ltd.
-// Licensed under the AGPLv3, see LICENCE file for details.
-//
-// +build windows
-
-package ssh
-
-// keyFingerprint returns the fingerprint and comment for the specified key.
-func keyFingerprint(key string) (fingerprint, comment string, err error) {
-	// TODO(wallyworld) 2013-12-09 bug # 1259368
-	// Implement this function on Windows.
-	panic("not implemented")
-}