FAQ
Reviewers: agl1, hanwen-google,

Message:
Hello agl@golang.org, hanwen@google.com (cc:
golang-dev@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go.crypto


Description:
go.crypto/ssh: fix certificate parsing/marshaling.

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

Affected files (+59, -20 lines):
    M ssh/agent.go
    M ssh/certs.go
    M ssh/common.go
    M ssh/keys.go
    M ssh/keys_test.go
    M ssh/server.go


Index: ssh/agent.go
===================================================================
--- a/ssh/agent.go
+++ b/ssh/agent.go
@@ -100,7 +100,7 @@

   // Key returns an agent's public key as one of the supported key or
certificate types.
   func (ak *AgentKey) Key() (PublicKey, error) {
- if key, _, ok := parsePubKey(ak.blob); ok {
+ if key, _, ok := ParsePublicKey(ak.blob); ok {
     return key, nil
    }
    return nil, errors.New("ssh: failed to parse key blob")
Index: ssh/certs.go
===================================================================
--- a/ssh/certs.go
+++ b/ssh/certs.go
@@ -60,6 +60,15 @@
    KeyAlgoECDSA521: CertAlgoECDSA521v01,
   }

+func pubToPrivAlgo(algo string) string {
+ for privAlgo, pubAlgo := range certAlgoNames {
+ if pubAlgo == algo {
+ return privAlgo
+ }
+ }
+ return ""
+}
+
   func (c *OpenSSHCertV01) PublicKeyAlgo() string {
    algo, ok := certAlgoNames[c.Key.PublicKeyAlgo()]
    if !ok {
@@ -87,12 +96,14 @@
     return
    }

- cert.Key, in, ok = ParsePublicKey(in)
+ privAlgo := pubToPrivAlgo(algo)
+ cert.Key, in, ok = parsePubKey(in, privAlgo)
    if !ok {
     return
    }

- if cert.Key.PrivateKeyAlgo() != algo {
+ // We test PublicKeyAlgo to make sure we don't use some weird sub-cert.
+ if cert.Key.PublicKeyAlgo() != privAlgo {
     ok = false
     return
    }
@@ -143,7 +154,7 @@
    if !ok {
     return
    }
- if cert.SignatureKey, _, ok = parsePubKey(sigKey); !ok {
+ if cert.SignatureKey, _, ok = ParsePublicKey(sigKey); !ok {
     return
    }

@@ -156,8 +167,7 @@
   }

   func (cert *OpenSSHCertV01) Marshal() []byte {
- pubKey := MarshalPublicKey(cert.Key)
-
+ pubKey := cert.Key.Marshal()
    sigKey := MarshalPublicKey(cert.SignatureKey)

    length := stringLength(len(cert.Nonce))
Index: ssh/common.go
===================================================================
--- a/ssh/common.go
+++ b/ssh/common.go
@@ -255,7 +255,7 @@
   // generating an authorized_keys or host_keys file.
   func MarshalPublicKey(key PublicKey) []byte {
    // See also RFC 4253 6.6.
- algoname := key.PrivateKeyAlgo()
+ algoname := key.PublicKeyAlgo()
    blob := key.Marshal()

    length := stringLength(len(algoname))
Index: ssh/keys.go
===================================================================
--- a/ssh/keys.go
+++ b/ssh/keys.go
@@ -25,14 +25,10 @@
    KeyAlgoECDSA521 = "ecdsa-sha2-nistp521"
   )

-// parsePubKey parses a public key according to RFC 4253, section 6.6.
-func parsePubKey(in []byte) (pubKey PublicKey, rest []byte, ok bool) {
- algo, in, ok := parseString(in)
- if !ok {
- return
- }
-
- switch string(algo) {
+// parsePubKey parses a public key of the given algorithm.
+// Use ParsePublicKey for keys with prepended algorithm.
+func parsePubKey(in []byte, algo string) (pubKey PublicKey, rest []byte,
ok bool) {
+ switch algo {
    case KeyAlgoRSA:
     return parseRSA(in)
    case KeyAlgoDSA:
@@ -40,7 +36,7 @@
    case KeyAlgoECDSA256, KeyAlgoECDSA384, KeyAlgoECDSA521:
     return parseECDSA(in)
    case CertAlgoRSAv01, CertAlgoDSAv01, CertAlgoECDSA256v01,
CertAlgoECDSA384v01, CertAlgoECDSA521v01:
- return parseOpenSSHCertV01(in, string(algo))
+ return parseOpenSSHCertV01(in, algo)
    }
    return nil, nil, false
   }
@@ -63,7 +59,7 @@
     return
    }
    key = key[:n]
- out, _, ok = parsePubKey(key)
+ out, _, ok = ParsePublicKey(key)
    if !ok {
     return nil, "", false
    }
@@ -173,9 +169,14 @@
   }

   // ParsePublicKey parses an SSH public key formatted for use in
-// the SSH wire protocol.
+// the SSH wire protocol according to RFC 4253, section 6.6.
   func ParsePublicKey(in []byte) (out PublicKey, rest []byte, ok bool) {
- return parsePubKey(in)
+ algo, in, ok := parseString(in)
+ if !ok {
+ return
+ }
+
+ return parsePubKey(in, string(algo))
   }

   // MarshalAuthorizedKey returns a byte stream suitable for inclusion
Index: ssh/keys_test.go
===================================================================
--- a/ssh/keys_test.go
+++ b/ssh/keys_test.go
@@ -1,10 +1,12 @@
   package ssh

   import (
+ "bytes"
    "crypto"
    "crypto/dsa"
    "crypto/rand"
    "crypto/rsa"
+ "encoding/base64"
    "reflect"
    "testing"
   )
@@ -58,3 +60,29 @@
   }

   // TODO(hanwen): test for ECDSA marshal.
+
+func TestParseCert(t *testing.T) {
+ // Cert generated by ssh-keygen 6.0p1 Debian-4.
+
b64data := "AAAAHHNzaC1yc2EtY2VydC12MDFAb3BlbnNzaC5jb20AAAAgb1srW/W3ZDjYAO45xLYAwzHBDLsJ4Ux6ICFIkTjb1LEAAAADAQABAAAAYQCkoR51poH0wE8w72cqSB8Sszx+vAhzcMdCO0wqHTj7UNENHWEXGrU0E0UQekD7U+yhkhtoyjbPOVIP7hNa6aRk/ezdh/iUnCIt4Jt1v3Z1h1P+hA4QuYFMHNB+rmjPwAcAAAAAAAAAAAAAAAEAAAAEdGVzdAAAAAAAAAAAAAAAAP//////////AAAAAAAAAIIAAAAVcGVybWl0LVgxMS1mb3J3YXJkaW5nAAAAAAAAABdwZXJtaXQtYWdlbnQtZm9yd2FyZGluZwAAAAAAAAAWcGVybWl0LXBvcnQtZm9yd2FyZGluZwAAAAAAAAAKcGVybWl0LXB0eQAAAAAAAAAOcGVybWl0LXVzZXItcmMAAAAAAAAAAAAAAHcAAAAHc3NoLXJzYQAAAAMBAAEAAABhANFS2kaktpSGc+CcmEKPyw9mJC4nZKxHKTgLVZeaGbFZOvJTNzBspQHdy7Q1uKSfktxpgjZnksiu/tFF9ngyY2KFoc+U88ya95IZUycBGCUbBQ8+bhDtw/icdDGQD5WnUwAAAG8AAAAHc3NoLXJzYQAAAGC8Y9Z2LQKhIhxf52773XaWrXdxP0t3GBVo4A10vUWiYoAGepr6rQIoGGXFxT4B9Gp+nEBJjOwKDXPrAevow0T9ca8gZN+0ykbhSrXLE5Ao48rqr3zP4O1/9P7e6gp0gw8="
+
+ data, err := base64.StdEncoding.DecodeString(b64data)
+ if err != nil {
+ t.Fatal(err)
+ }
+ key, rest, ok := ParsePublicKey(data)
+ if !ok {
+ t.Fatalf("could not parse certificate")
+ }
+ if len(rest) > 0 {
+ t.Errorf("rest: got %#v, want empty", rest)
+ }
+ cert, ok := key.(*OpenSSHCertV01)
+ if !ok {
+ t.Fatalf("got %#v, want *OpenSSHCertV01", key)
+ }
+
+ marshaled := MarshalPublicKey(key)
+ if !bytes.Equal(data, marshaled) {
+ t.Errorf("marshaled certificate does not match original: got %#v,
want %#v", marshaled, data)
+ }
+}
Index: ssh/server.go
===================================================================
--- a/ssh/server.go
+++ b/ssh/server.go
@@ -599,7 +599,7 @@
        break
       }
       signedData := buildDataSignedForAuth(H, userAuthReq, algoBytes, pubKey)
- key, _, ok := parsePubKey(pubKey)
+ key, _, ok := ParsePublicKey(pubKey)
       if !ok {
        return ParseError{msgUserAuthRequest}
       }


--

---
You received this message because you are subscribed to the Google Groups "golang-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Search Discussions

  • Agl at Sep 18, 2013 at 7:27 pm
    LGTM, will will wait for hanwen before landing.

    I'd encourage a longer change description.

    https://codereview.appspot.com/13272055/

    --

    ---
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.
  • JP Sugarbroad at Sep 18, 2013 at 7:33 pm

    On Wed, Sep 18, 2013 at 12:27 PM, wrote:
    I'd encourage a longer change description.
    Sure, what were you looking for? I could add a statement that I added
    a test for cert parsing/marshaling, or a description of the minor API
    change?

    - JP

    --

    ---
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.
  • Adam Langley at Sep 18, 2013 at 7:44 pm

    On Wed, Sep 18, 2013 at 3:33 PM, JP Sugarbroad wrote:
    Sure, what were you looking for? I could add a statement that I added
    a test for cert parsing/marshaling, or a description of the minor API
    change?
    I'd just look for a quick discussion of what was wrong and how this fixes it.


    Cheers

    AGL

    --

    ---
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.
  • JP Sugarbroad at Sep 18, 2013 at 8:01 pm

    On Wed, Sep 18, 2013 at 12:44 PM, Adam Langley wrote:
    I'd just look for a quick discussion of what was wrong and how this fixes it.
    Done.

    --

    ---
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.
  • Hanwen at Sep 18, 2013 at 8:02 pm
    LGTM

    Looks good, modulo nits.

    Thanks for adding test coverage; that was dearly needed.


    https://codereview.appspot.com/13272055/diff/6001/ssh/certs.go
    File ssh/certs.go (right):

    https://codereview.appspot.com/13272055/diff/6001/ssh/certs.go#newcode69
    ssh/certs.go:69: return ""
    return algo here? Now it only works for cert algorithms.

    https://codereview.appspot.com/13272055/diff/6001/ssh/keys_test.go
    File ssh/keys_test.go (right):

    https://codereview.appspot.com/13272055/diff/6001/ssh/keys_test.go#newcode65
    ssh/keys_test.go:65: // Cert generated by ssh-keygen 6.0p1 Debian-4.
    can you quote the command line?

    https://codereview.appspot.com/13272055/diff/6001/ssh/keys_test.go#newcode70
    ssh/keys_test.go:70: t.Fatal(err)
    mention the failing function for context.

    https://codereview.appspot.com/13272055/diff/6001/ssh/keys_test.go#newcode86
    ssh/keys_test.go:86: t.Errorf("marshaled certificate does not match
    original: got %#v, want %#v", marshaled, data)
    %#v will print
    []byte{0x1, 0x3, ... }

    I'd use %q or %x for data. I like %q because the marshaled strings stand
    out.

    The same in other calls above.

    https://codereview.appspot.com/13272055/

    --

    ---
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.
  • Jpsugar at Sep 18, 2013 at 8:11 pm
    https://codereview.appspot.com/13272055/diff/6001/ssh/certs.go
    File ssh/certs.go (right):

    https://codereview.appspot.com/13272055/diff/6001/ssh/certs.go#newcode69
    ssh/certs.go:69: return ""
    On 2013/09/18 20:02:39, hanwen-google wrote:
    return algo here? Now it only works for cert algorithms.
    It's only supposed to work for cert algorithms -- anything else is an
    error. Changed the name and added a comment.

    https://codereview.appspot.com/13272055/diff/6001/ssh/keys_test.go
    File ssh/keys_test.go (right):

    https://codereview.appspot.com/13272055/diff/6001/ssh/keys_test.go#newcode65
    ssh/keys_test.go:65: // Cert generated by ssh-keygen 6.0p1 Debian-4.
    On 2013/09/18 20:02:39, hanwen-google wrote:
    can you quote the command line?
    Done.

    https://codereview.appspot.com/13272055/diff/6001/ssh/keys_test.go#newcode70
    ssh/keys_test.go:70: t.Fatal(err)
    On 2013/09/18 20:02:39, hanwen-google wrote:
    mention the failing function for context.
    Done.

    https://codereview.appspot.com/13272055/diff/6001/ssh/keys_test.go#newcode86
    ssh/keys_test.go:86: t.Errorf("marshaled certificate does not match
    original: got %#v, want %#v", marshaled, data)
    On 2013/09/18 20:02:39, hanwen-google wrote:
    %#v will print
    []byte{0x1, 0x3, ... }
    I'd use %q or %x for data. I like %q because the marshaled strings
    stand out.
    The same in other calls above.
    Done.

    https://codereview.appspot.com/13272055/

    --

    ---
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedSep 18, '13 at 7:18p
activeSep 18, '13 at 8:11p
posts7
users3
websitegolang.org

3 users in discussion

Jpsugar: 4 posts Adam Langley: 2 posts Hanwen: 1 post

People

Translate

site design / logo © 2021 Grokbase