Merge lp:~axwalk/juju-core/ssh-authorized-keys-gocrypto into lp:~go-bot/juju-core/trunk

[Andrew Wilkins]

utils/ssh: use go.crypto/ssh.ParseAuthorizedKey

There's a function in go.crypto/ssh that we
can use for parsing authorized_key lines. We
should use it.

https://codereview.appspot.com/58540043/

utils/ssh: use go.crypto/ssh.ParseAuthorizedKey

There's a function in go.crypto/ssh that we
can use for parsing authorized_key lines. We
should use it.

https://codereview.appspot.com/58540043/

[Andrew Wilkins (axwalk)]

Reviewers: mp+203878_code.launchpad.net,

Message:
Please take a look.

Description:
utils/ssh: use go.crypto/ssh.ParseAuthorizedKey

There's a function in go.crypto/ssh that we
can use for parsing authorized_key lines. We
should use it.

https://code.launchpad.net/~axwalk/juju-core/ssh-authorized-keys-gocrypto/+merge/203878

(do not edit description out of merge proposal)

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

Affected files (+25, -116 lines):
   A [revision details]
   M utils/ssh/authorisedkeys.go
   M utils/ssh/authorisedkeys_test.go
   M utils/ssh/fingerprint_test.go

[Ian Booth (wallyworld)]

LGTM

https://codereview.appspot.com/58540043/

=== modified file 'utils/ssh/authorisedkeys.go'
--- utils/ssh/authorisedkeys.go	2014-01-28 04:58:43 +0000
+++ utils/ssh/authorisedkeys.go	2014-01-30 06:16:19 +0000
@@ -4,7 +4,6 @@
 package ssh
 
 import (
-	"encoding/base64"
 	"fmt"
 	"io/ioutil"
 	"os"
@@ -15,6 +14,7 @@
 	"strings"
 	"sync"
 
+	"code.google.com/p/go.crypto/ssh"
 	"github.com/loggo/loggo"
 
 	"launchpad.net/juju-core/utils"
@@ -34,108 +34,21 @@
 	authKeysFile = "authorized_keys"
 )
 
-// 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
-	}
+	key, comment, _, _, ok := ssh.ParseAuthorizedKey([]byte(line))
+	if !ok {
+		return nil, fmt.Errorf("invalid authorized_key %q", line)
+	}
+	keyBytes := ssh.MarshalPublicKey(key)
 	return &AuthorisedKey{
-		KeyType: keytype,
 		Key:     keyBytes,
 		Comment: comment,
 	}, nil

=== modified file 'utils/ssh/authorisedkeys_test.go'
--- utils/ssh/authorisedkeys_test.go	2014-01-10 02:42:02 +0000
+++ utils/ssh/authorisedkeys_test.go	2014-01-30 06:16:19 +0000
@@ -5,6 +5,7 @@
 
 import (
 	"encoding/base64"
+	"strings"
 	stdtesting "testing"
 
 	gc "launchpad.net/gocheck"
@@ -236,41 +237,35 @@
 	}
 }
 
-func b64(s string) string {
-	return base64.StdEncoding.EncodeToString([]byte(s))
+func b64decode(c *gc.C, s string) []byte {
+	b, err := base64.StdEncoding.DecodeString(s)
+	c.Assert(err, gc.IsNil)
+	return b
 }
 
 func (s *AuthorisedKeysKeysSuite) TestParseAuthorisedKey(c *gc.C) {
 	for i, test := range []struct {
 		line    string
-		keytype string
-		key     string
+		key     []byte
 		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",
+		line: sshtesting.ValidKeyOne.Key,
+		key:  b64decode(c, strings.Fields(sshtesting.ValidKeyOne.Key)[1]),
+	}, {
+		line:    sshtesting.ValidKeyOne.Key + " a b c",
+		key:     b64decode(c, strings.Fields(sshtesting.ValidKeyOne.Key)[1]),
 		comment: "a b c",
 	}, {
 		line: "ssh-xsa blah",
-		err:  "invalid keytype \"ssh-xsa\" in line \"ssh-xsa blah\"",
+		err:  "invalid authorized_key \"ssh-xsa blah\"",
 	}, {
 		// options should be skipped
-		line:    `no-pty,principals="\"",command="\!" ssh-rsa ` + b64("blah"),
-		keytype: "ssh-rsa",
-		key:     "blah",
+		line: `no-pty,principals="\"",command="\!" ` + sshtesting.ValidKeyOne.Key,
+		key:  b64decode(c, strings.Fields(sshtesting.ValidKeyOne.Key)[1]),
 	}, {
 		line: "ssh-rsa",
-		err:  "malformed line: \"ssh-rsa\"",
+		err:  "invalid authorized_key \"ssh-rsa\"",
 	}} {
 		c.Logf("test %d: %s", i, test.line)
 		ak, err := ssh.ParseAuthorisedKey(test.line)
@@ -279,8 +274,7 @@
 		} 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.Key, gc.DeepEquals, test.key)
 			c.Assert(ak.Comment, gc.Equals, test.comment)
 		}
 	}

=== modified file 'utils/ssh/fingerprint_test.go'
--- utils/ssh/fingerprint_test.go	2014-01-10 02:42:02 +0000
+++ utils/ssh/fingerprint_test.go	2014-01-30 06:16:19 +0000
@@ -32,5 +32,5 @@
 
 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"`)
+	c.Assert(err, gc.ErrorMatches, `generating key fingerprint: invalid authorized_key "invalid key"`)
 }