|
|
Descriptionopenpgp/packet: Parse and serialize ECC key material (RFC 6637).
Support ECDSA signature verification.
Patch Set 1 #Patch Set 2 : diff -r 1e7a3e301825 https://code.google.com/p/go.crypto #
Total comments: 54
Patch Set 3 : diff -r 5c2f63878818 https://code.google.com/p/go.crypto #
Total comments: 4
Patch Set 4 : diff -r 5c2f63878818 https://code.google.com/p/go.crypto #
MessagesTotal messages: 9
https://codereview.appspot.com/13141044/diff/3001/openpgp/packet/public_key.go File openpgp/packet/public_key.go (right): https://codereview.appspot.com/13141044/diff/3001/openpgp/packet/public_key.g... openpgp/packet/public_key.go:27: var curveOidP256 []byte = []byte{0x2A, 0x86, 0x48, 0xCE, 0x3D, 0x03, 0x01, 0x07} var ( ... ) https://codereview.appspot.com/13141044/diff/3001/openpgp/packet/public_key.g... openpgp/packet/public_key.go:27: var curveOidP256 []byte = []byte{0x2A, 0x86, 0x48, 0xCE, 0x3D, 0x03, 0x01, 0x07} s/Oid/OID/ to match crypto/x509. https://codereview.appspot.com/13141044/diff/3001/openpgp/packet/public_key.g... openpgp/packet/public_key.go:31: var curveOidP521 []byte = []byte{0x2B, 0x81, 0x04, 0x00, 0x23} const maxOIDLength = 8 and use it in the functions below. https://codereview.appspot.com/13141044/diff/3001/openpgp/packet/public_key.g... openpgp/packet/public_key.go:33: type ecFields struct { needs comment. ecFields -> ecdsaKey? https://codereview.appspot.com/13141044/diff/3001/openpgp/packet/public_key.g... openpgp/packet/public_key.go:37: RFC references? https://codereview.appspot.com/13141044/diff/3001/openpgp/packet/public_key.g... openpgp/packet/public_key.go:40: _, err = readFull(r, buf[:1]) merge if https://codereview.appspot.com/13141044/diff/3001/openpgp/packet/public_key.g... openpgp/packet/public_key.go:49: _, err = readFull(r, buf[:oidLen]) swap these two lines and pass "oid" as the second argument to readFull. https://codereview.appspot.com/13141044/diff/3001/openpgp/packet/public_key.g... openpgp/packet/public_key.go:64: buf := make([]byte, 8) why not make the buffer one byte longer and save a write? https://codereview.appspot.com/13141044/diff/3001/openpgp/packet/public_key.g... openpgp/packet/public_key.go:71: _, err = w.Write(buf[:len(f.oid)]) The existing code may be sloppy about this in other places, but probably good to test err after the Write. https://codereview.appspot.com/13141044/diff/3001/openpgp/packet/public_key.g... openpgp/packet/public_key.go:86: x, y := elliptic.Unmarshal(c, f.p.bytes) should test whether x == nil after this call. https://codereview.appspot.com/13141044/diff/3001/openpgp/packet/public_key.g... openpgp/packet/public_key.go:97: type ecdhFields struct { needs comment. ecdhFields -> ecdhKey? https://codereview.appspot.com/13141044/diff/3001/openpgp/packet/public_key.g... openpgp/packet/public_key.go:104: _, err = readFull(r, buf[:]) [:] not needed if you're using a slice rather than an array. https://codereview.appspot.com/13141044/diff/3001/openpgp/packet/public_key.g... openpgp/packet/public_key.go:105: if err != nil { merge if. https://codereview.appspot.com/13141044/diff/3001/openpgp/packet/public_key.g... openpgp/packet/public_key.go:110: _, err = readFull(r, buf[:]) ditto. https://codereview.appspot.com/13141044/diff/3001/openpgp/packet/public_key.g... openpgp/packet/public_key.go:111: if err != nil { merge if. https://codereview.appspot.com/13141044/diff/3001/openpgp/packet/public_key.g... openpgp/packet/public_key.go:114: if kdfLen < 3 { this can be moved prior to readFull I think. https://codereview.appspot.com/13141044/diff/3001/openpgp/packet/public_key.g... openpgp/packet/public_key.go:127: buf := make([]byte, 4) RFC reference? https://codereview.appspot.com/13141044/diff/3001/openpgp/packet/public_key.g... openpgp/packet/public_key.go:215: if err != nil { merge if. https://codereview.appspot.com/13141044/diff/3001/openpgp/packet/public_key.g... openpgp/packet/public_key.go:222: if err != nil { merge if. https://codereview.appspot.com/13141044/diff/3001/openpgp/packet/public_key.g... openpgp/packet/public_key.go:227: if err != nil { merge if. https://codereview.appspot.com/13141044/diff/3001/openpgp/packet/public_key.g... openpgp/packet/public_key.go:230: pk.PublicKey, err = pk.ec.newECDSA() seems odd to be creating an ECDSA key here rather than ECDH. Is this because there's no type for ECDH? https://codereview.appspot.com/13141044/diff/3001/openpgp/packet/public_key.g... openpgp/packet/public_key.go:427: if err != nil { merge if. https://codereview.appspot.com/13141044/diff/3001/openpgp/packet/public_key.g... openpgp/packet/public_key.go:478: ecdsaPublicKey, _ := pk.PublicKey.(*ecdsa.PublicKey) , _ not needed, I think. https://codereview.appspot.com/13141044/diff/3001/openpgp/packet/public_key_t... File openpgp/packet/public_key_test.go (right): https://codereview.appspot.com/13141044/diff/3001/openpgp/packet/public_key_t... openpgp/packet/public_key_test.go:101: w := bytes.NewBuffer(nil) var w bytes.Buffer https://codereview.appspot.com/13141044/diff/3001/openpgp/packet/public_key_t... openpgp/packet/public_key_test.go:190: w = bytes.NewBuffer(nil) w.Reset() https://codereview.appspot.com/13141044/diff/3001/openpgp/packet/public_key_t... openpgp/packet/public_key_test.go:207: const ecc384PubArmor = ` For consistency, just write this as hex: 99006f044d53059213052b81040022030304f6b8c5aced5b84ef9f4a209db2e4a9dfb70d28cb8c10ecd57674a9fa5a67389942b62d5e51367df4c7bfd3f8e500feecf07ed265a621a8ebbbe53e947ec78c677eba143bd1533c2b350e1c29f82313e1e1108eba063be1e64b10e6950e799c2db42465635f6473615f64685f333834203c6f70656e70677040627261696e6875622e6f72673e8900cb04101309005305024d530592301480000000002000077072656665727265642d656d61696c2d656e636f64696e67407067702e636f6d7067706d696d65040b090807021901051b03000000021602051e010000000415090a08000a0910098033880f54719fca2b0180aa37350968bd5f115afd8ce7bc7b103822152dbff06d0afcda835329510905b98cb469ba208faab87c7412b799e7b633017f58364ea480e8a1a3f253a0c5f22c446e8be9a9fce6210136ee30811abbd49139de28b5bdf8dc36d06ae748579e9ff503b90073044d53059212052b810400220303042faa84024a20b6735c4897efa5bfb41bf85b7eefeab5ca0cb9ffc8ea04a46acb25534a577694f9e25340a4ab5223a9dd1eda530c8aa2e6718db10d7e672558c7736fe09369ea5739a2a3554bf16d41faa50562f11c6d39bbd5dffb6b9a9ec9180301090989008404181309000c05024d530592051b0c000000000a0910098033880f54719f80970180eee7a6d8fcee41ee4f9289df17f9bcf9d955dca25c583b94336f3a2b2d4986dc5cf417b8d2dc86f741a9e1a6d236c0e3017d1c76575458a0cfb93ae8a2b274fcc65ceecd7a91eec83656ba13219969f06945b48c56bd04152c3a0553c5f2f4bd1267
Sign in to reply to this message.
Note: you've uploaded a new patch set to this change, but not published any replies. You may have pending comments.
Sign in to reply to this message.
On 2013/08/29 14:39:47, agl1 wrote: > Note: you've uploaded a new patch set to this change, but not published any > replies. You may have pending comments. Thanks, I'm almost ready for another look. It was getting last night, wanted to review this morning before submitting.
Sign in to reply to this message.
PTAL https://codereview.appspot.com/13141044/diff/3001/openpgp/packet/public_key.go File openpgp/packet/public_key.go (right): https://codereview.appspot.com/13141044/diff/3001/openpgp/packet/public_key.g... openpgp/packet/public_key.go:27: var curveOidP256 []byte = []byte{0x2A, 0x86, 0x48, 0xCE, 0x3D, 0x03, 0x01, 0x07} On 2013/08/26 14:55:20, agl1 wrote: > var ( > ... > ) Done. https://codereview.appspot.com/13141044/diff/3001/openpgp/packet/public_key.g... openpgp/packet/public_key.go:27: var curveOidP256 []byte = []byte{0x2A, 0x86, 0x48, 0xCE, 0x3D, 0x03, 0x01, 0x07} On 2013/08/26 14:55:20, agl1 wrote: > s/Oid/OID/ to match crypto/x509. Done. https://codereview.appspot.com/13141044/diff/3001/openpgp/packet/public_key.g... openpgp/packet/public_key.go:31: var curveOidP521 []byte = []byte{0x2B, 0x81, 0x04, 0x00, 0x23} On 2013/08/26 14:55:20, agl1 wrote: > const maxOIDLength = 8 and use it in the functions below. Done. https://codereview.appspot.com/13141044/diff/3001/openpgp/packet/public_key.g... openpgp/packet/public_key.go:33: type ecFields struct { On 2013/08/26 14:55:20, agl1 wrote: > needs comment. ecFields -> ecdsaKey? Done. https://codereview.appspot.com/13141044/diff/3001/openpgp/packet/public_key.g... openpgp/packet/public_key.go:37: On 2013/08/26 14:55:20, agl1 wrote: > RFC references? Done. https://codereview.appspot.com/13141044/diff/3001/openpgp/packet/public_key.g... openpgp/packet/public_key.go:40: _, err = readFull(r, buf[:1]) On 2013/08/26 14:55:20, agl1 wrote: > merge if Done. https://codereview.appspot.com/13141044/diff/3001/openpgp/packet/public_key.g... openpgp/packet/public_key.go:49: _, err = readFull(r, buf[:oidLen]) On 2013/08/26 14:55:20, agl1 wrote: > swap these two lines and pass "oid" as the second argument to readFull. Done. https://codereview.appspot.com/13141044/diff/3001/openpgp/packet/public_key.g... openpgp/packet/public_key.go:64: buf := make([]byte, 8) On 2013/08/26 14:55:20, agl1 wrote: > why not make the buffer one byte longer and save a write? Done. https://codereview.appspot.com/13141044/diff/3001/openpgp/packet/public_key.g... openpgp/packet/public_key.go:71: _, err = w.Write(buf[:len(f.oid)]) On 2013/08/26 14:55:20, agl1 wrote: > The existing code may be sloppy about this in other places, but probably good to > test err after the Write. Done. https://codereview.appspot.com/13141044/diff/3001/openpgp/packet/public_key.g... openpgp/packet/public_key.go:86: x, y := elliptic.Unmarshal(c, f.p.bytes) On 2013/08/26 14:55:20, agl1 wrote: > should test whether x == nil after this call. Done. https://codereview.appspot.com/13141044/diff/3001/openpgp/packet/public_key.g... openpgp/packet/public_key.go:97: type ecdhFields struct { On 2013/08/26 14:55:20, agl1 wrote: > needs comment. ecdhFields -> ecdhKey? Agree that 'ecdhFields' is too vague. How about 'ecdhKdf', since it just stores the KDF used in combination with the ECDSA key for encryption? https://codereview.appspot.com/13141044/diff/3001/openpgp/packet/public_key.g... openpgp/packet/public_key.go:104: _, err = readFull(r, buf[:]) On 2013/08/26 14:55:20, agl1 wrote: > [:] not needed if you're using a slice rather than an array. Done. https://codereview.appspot.com/13141044/diff/3001/openpgp/packet/public_key.g... openpgp/packet/public_key.go:105: if err != nil { On 2013/08/26 14:55:20, agl1 wrote: > merge if. Done. https://codereview.appspot.com/13141044/diff/3001/openpgp/packet/public_key.g... openpgp/packet/public_key.go:110: _, err = readFull(r, buf[:]) On 2013/08/26 14:55:20, agl1 wrote: > ditto. Done. https://codereview.appspot.com/13141044/diff/3001/openpgp/packet/public_key.g... openpgp/packet/public_key.go:111: if err != nil { On 2013/08/26 14:55:20, agl1 wrote: > merge if. Done. https://codereview.appspot.com/13141044/diff/3001/openpgp/packet/public_key.g... openpgp/packet/public_key.go:114: if kdfLen < 3 { On 2013/08/26 14:55:20, agl1 wrote: > this can be moved prior to readFull I think. Done. https://codereview.appspot.com/13141044/diff/3001/openpgp/packet/public_key.g... openpgp/packet/public_key.go:127: buf := make([]byte, 4) On 2013/08/26 14:55:20, agl1 wrote: > RFC reference? Done. https://codereview.appspot.com/13141044/diff/3001/openpgp/packet/public_key.g... openpgp/packet/public_key.go:215: if err != nil { On 2013/08/26 14:55:20, agl1 wrote: > merge if. Done. https://codereview.appspot.com/13141044/diff/3001/openpgp/packet/public_key.g... openpgp/packet/public_key.go:222: if err != nil { On 2013/08/26 14:55:20, agl1 wrote: > merge if. Done. https://codereview.appspot.com/13141044/diff/3001/openpgp/packet/public_key.g... openpgp/packet/public_key.go:227: if err != nil { On 2013/08/26 14:55:20, agl1 wrote: > merge if. Done. https://codereview.appspot.com/13141044/diff/3001/openpgp/packet/public_key.g... openpgp/packet/public_key.go:230: pk.PublicKey, err = pk.ec.newECDSA() On 2013/08/26 14:55:20, agl1 wrote: > seems odd to be creating an ECDSA key here rather than ECDH. Is this because > there's no type for ECDH? Correct. My understanding of ECDH (and I am still getting up to speed on the details), is that it uses the same kind of ECDSA public key in a Diffie-Hellman computation. The public key-derived secret is used with the KDF method to derive a symmetric key (which is then used to wrap a typical OpenPGP session key for message encryption). https://codereview.appspot.com/13141044/diff/3001/openpgp/packet/public_key.g... openpgp/packet/public_key.go:427: if err != nil { On 2013/08/26 14:55:20, agl1 wrote: > merge if. Done. https://codereview.appspot.com/13141044/diff/3001/openpgp/packet/public_key.g... openpgp/packet/public_key.go:478: ecdsaPublicKey, _ := pk.PublicKey.(*ecdsa.PublicKey) On 2013/08/26 14:55:20, agl1 wrote: > , _ not needed, I think. Done. https://codereview.appspot.com/13141044/diff/3001/openpgp/packet/public_key_t... File openpgp/packet/public_key_test.go (right): https://codereview.appspot.com/13141044/diff/3001/openpgp/packet/public_key_t... openpgp/packet/public_key_test.go:101: w := bytes.NewBuffer(nil) On 2013/08/26 14:55:20, agl1 wrote: > var w bytes.Buffer Done. https://codereview.appspot.com/13141044/diff/3001/openpgp/packet/public_key_t... openpgp/packet/public_key_test.go:190: w = bytes.NewBuffer(nil) On 2013/08/26 14:55:20, agl1 wrote: > w.Reset() Done. https://codereview.appspot.com/13141044/diff/3001/openpgp/packet/public_key_t... openpgp/packet/public_key_test.go:207: const ecc384PubArmor = ` On 2013/08/26 14:55:20, agl1 wrote: > For consistency, just write this as hex: > > 99006f044d53059213052b81040022030304f6b8c5aced5b84ef9f4a209db2e4a9dfb70d28cb8c10ecd57674a9fa5a67389942b62d5e51367df4c7bfd3f8e500feecf07ed265a621a8ebbbe53e947ec78c677eba143bd1533c2b350e1c29f82313e1e1108eba063be1e64b10e6950e799c2db42465635f6473615f64685f333834203c6f70656e70677040627261696e6875622e6f72673e8900cb04101309005305024d530592301480000000002000077072656665727265642d656d61696c2d656e636f64696e67407067702e636f6d7067706d696d65040b090807021901051b03000000021602051e010000000415090a08000a0910098033880f54719fca2b0180aa37350968bd5f115afd8ce7bc7b103822152dbff06d0afcda835329510905b98cb469ba208faab87c7412b799e7b633017f58364ea480e8a1a3f253a0c5f22c446e8be9a9fce6210136ee30811abbd49139de28b5bdf8dc36d06ae748579e9ff503b90073044d53059212052b810400220303042faa84024a20b6735c4897efa5bfb41bf85b7eefeab5ca0cb9ffc8ea04a46acb25534a577694f9e25340a4ab5223a9dd1eda530c8aa2e6718db10d7e672558c7736fe09369ea5739a2a3554bf16d41faa50562f11c6d39bbd5dffb6b9a9ec9180301090989008404181309000c05024d530592051b0c000000000a0910098033880f54719f80970180eee7a6d8fcee41ee4f9289df17f9bcf9d955dca25c583b94336f3a2b2d4986dc5cf417b8d2dc86f741a9e1a6d236c0e3017d1c76575458a0cfb93ae8a2b274fcc65ceecd7a91eec83656ba13219969f06945b48c56bd04152c3a0553c5f2f4bd1267 Done.
Sign in to reply to this message.
LGTM with nits. https://codereview.appspot.com/13141044/diff/3001/openpgp/packet/public_key.go File openpgp/packet/public_key.go (right): https://codereview.appspot.com/13141044/diff/3001/openpgp/packet/public_key.g... openpgp/packet/public_key.go:230: pk.PublicKey, err = pk.ec.newECDSA() On 2013/08/29 15:58:49, cmars wrote: > On 2013/08/26 14:55:20, agl1 wrote: > > seems odd to be creating an ECDSA key here rather than ECDH. Is this because > > there's no type for ECDH? > > Correct. My understanding of ECDH (and I am still getting up to speed on the > details), is that it uses the same kind of ECDSA public key in a Diffie-Hellman > computation. The public key-derived secret is used with the KDF method to derive > a symmetric key (which is then used to wrap a typical OpenPGP session key for > message encryption). That's correct. Both DSA and ElGamal (which is essentially what's being done here, but in an elliptic, not multiplicative group) just use group elements. So an ECDSA key stores what you need. Perhaps "// The ECDH key is stored in an ecdsa.PublicKey for convenience." https://codereview.appspot.com/13141044/diff/23001/openpgp/packet/public_key.go File openpgp/packet/public_key.go (right): https://codereview.appspot.com/13141044/diff/23001/openpgp/packet/public_key.... openpgp/packet/public_key.go:29: OIDCurveP256 []byte = []byte{0x2A, 0x86, 0x48, 0xCE, 0x3D, 0x03, 0x01, 0x07} if OID is first then I think it's 'oid' since these don't need to be exported, right? https://codereview.appspot.com/13141044/diff/23001/openpgp/packet/public_key.... openpgp/packet/public_key.go:59: _, err = readFull(r, oid[:oidLen]) "[:oidLen]" not needed.
Sign in to reply to this message.
PTAL https://codereview.appspot.com/13141044/diff/3001/openpgp/packet/public_key.go File openpgp/packet/public_key.go (right): https://codereview.appspot.com/13141044/diff/3001/openpgp/packet/public_key.g... openpgp/packet/public_key.go:230: pk.PublicKey, err = pk.ec.newECDSA() On 2013/08/29 16:40:23, agl1 wrote: > On 2013/08/29 15:58:49, cmars wrote: > > On 2013/08/26 14:55:20, agl1 wrote: > > > seems odd to be creating an ECDSA key here rather than ECDH. Is this because > > > there's no type for ECDH? > > > > Correct. My understanding of ECDH (and I am still getting up to speed on the > > details), is that it uses the same kind of ECDSA public key in a > Diffie-Hellman > > computation. The public key-derived secret is used with the KDF method to > derive > > a symmetric key (which is then used to wrap a typical OpenPGP session key for > > message encryption). > > That's correct. Both DSA and ElGamal (which is essentially what's being done > here, but in an elliptic, not multiplicative group) just use group elements. So > an ECDSA key stores what you need. Perhaps "// The ECDH key is stored in an > ecdsa.PublicKey for convenience." Done. https://codereview.appspot.com/13141044/diff/23001/openpgp/packet/public_key.go File openpgp/packet/public_key.go (right): https://codereview.appspot.com/13141044/diff/23001/openpgp/packet/public_key.... openpgp/packet/public_key.go:29: OIDCurveP256 []byte = []byte{0x2A, 0x86, 0x48, 0xCE, 0x3D, 0x03, 0x01, 0x07} On 2013/08/29 16:40:23, agl1 wrote: > if OID is first then I think it's 'oid' since these don't need to be exported, > right? Done. https://codereview.appspot.com/13141044/diff/23001/openpgp/packet/public_key.... openpgp/packet/public_key.go:59: _, err = readFull(r, oid[:oidLen]) On 2013/08/29 16:40:23, agl1 wrote: > "[:oidLen]" not needed. Done.
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=4dbd0e989373&repo=crypto *** openpgp/packet: Parse and serialize ECC key material (RFC 6637). Support ECDSA signature verification. R=agl CC=golang-dev https://codereview.appspot.com/13141044 Committer: Adam Langley <agl@golang.org>
Sign in to reply to this message.
|