openpgp: improve parser resilience & flexibility, add P...

openpgp: improve parser resilience & flexibility, add PublicKey.BitLength() These are improvements I've made as necessary to develop Hockeypuck, an OpenPGP keyserver (https://launchpad.net/hockeypuck). The max armor line length was increased to 96 because some keyservers (SKS) will output armor with lines greater than 64 (the prior max). I've exposed packet.ReadEntity to support stream-parsing, useful for large SKS dump files. ReadKeyRing attempts to recover in the event of malformed data. Because many packet types are not yet supported, I added OpaquePacket to capture unsupported types for offline storage and later reparsing.

[agl1, 2012-12-11 15:08:41]

https://codereview.appspot.com/6927044/diff/2001/openpgp/keys.go File openpgp/keys.go (right): https://codereview.appspot.com/6927044/diff/2001/openpgp/keys.go#newcode209 openpgp/keys.go:209: // Skip unreadable badly-formatted keys comma after 'unreadable'. https://codereview.appspot.com/6927044/diff/2001/openpgp/packet/opaque.go File openpgp/packet/opaque.go (right): https://codereview.appspot.com/6927044/diff/2001/openpgp/packet/opaque.go#new... openpgp/packet/opaque.go:9: // An opaque packet represents an OpenPGP packet as raw unparsed s/An opaque packet /OpaquePacket / https://codereview.appspot.com/6927044/diff/2001/openpgp/packet/opaque.go#new... openpgp/packet/opaque.go:9: // An opaque packet represents an OpenPGP packet as raw unparsed comma after 'raw'. https://codereview.appspot.com/6927044/diff/2001/openpgp/packet/opaque.go#new... openpgp/packet/opaque.go:24: buf := bytes.NewBuffer([]byte{}) This looks like http://golang.org/pkg/io/ioutil/#ReadAll https://codereview.appspot.com/6927044/diff/2001/openpgp/packet/opaque.go#new... openpgp/packet/opaque.go:30: // Serialize the packet to a writer in its original form including comma after 'form'. https://codereview.appspot.com/6927044/diff/2001/openpgp/packet/opaque.go#new... openpgp/packet/opaque.go:30: // Serialize the packet to a writer in its original form including Serialize marshals the packet... https://codereview.appspot.com/6927044/diff/2001/openpgp/packet/opaque.go#new... openpgp/packet/opaque.go:40: // Attempt to parse the opaque contents into a structure supported // Parse attempts to parse... https://codereview.appspot.com/6927044/diff/2001/openpgp/packet/opaque.go#new... openpgp/packet/opaque.go:42: // will still be an opaque packet. will be op. https://codereview.appspot.com/6927044/diff/2001/openpgp/packet/opaque.go#new... openpgp/packet/opaque.go:44: hdr := bytes.NewBuffer([]byte{}) s/[]byte{}/nil/ https://codereview.appspot.com/6927044/diff/2001/openpgp/packet/opaque.go#new... openpgp/packet/opaque.go:49: return Read(io.MultiReader(bytes.NewBuffer(hdr.Bytes()), bytes.NewBuffer(op.Contents))) I'm not sure why you're creating a new bytes.Buffer for the first argument here. https://codereview.appspot.com/6927044/diff/2001/openpgp/packet/opaque.go#new... openpgp/packet/opaque.go:52: // Reads OpaquePackets from an io.Reader. OpaqueReader reads OpaquePackets from an io.Reader. https://codereview.appspot.com/6927044/diff/2001/openpgp/packet/opaque.go#new... openpgp/packet/opaque.go:75: // Read a subpacket from the opaque contents. OpaqueSubpacket represents an unparsed OpenPGP subpacket, as found in signature and user attribute packets. https://codereview.appspot.com/6927044/diff/2001/openpgp/packet/opaque.go#new... openpgp/packet/opaque.go:83: func OpaqueSubpackets(contents []byte) (result []*OpaqueSubpacket, err error) { // OpaqueSubpackets ... https://codereview.appspot.com/6927044/diff/2001/openpgp/packet/opaque.go#new... openpgp/packet/opaque.go:84: result = []*OpaqueSubpacket{} var result []*OpaqueSubpacket https://codereview.appspot.com/6927044/diff/2001/openpgp/packet/opaque.go#new... openpgp/packet/opaque.go:86: subHeaderLen uint32 you're trying too hard to use named return values here. https://codereview.appspot.com/6927044/diff/2001/openpgp/packet/packet.go File openpgp/packet/packet.go (right): https://codereview.appspot.com/6927044/diff/2001/openpgp/packet/packet.go#new... openpgp/packet/packet.go:345: backup := bytes.NewBuffer([]byte{}) the argument can just be nil, I think. https://codereview.appspot.com/6927044/diff/2001/openpgp/packet/packet.go#new... openpgp/packet/packet.go:348: var retry bool rather than use this bool, I think you can just: switch err.(type) { case errors.UnsupportedError, errors.UnkownPacketTypeError: p = &OpaquePacket{...} } although maybe that doesn't work for some reason. https://codereview.appspot.com/6927044/diff/2001/openpgp/packet/public_key.go File openpgp/packet/public_key.go (right): https://codereview.appspot.com/6927044/diff/2001/openpgp/packet/public_key.go... openpgp/packet/public_key.go:416: // Return the bit length for the given public key. // BitLength returns the bit length for the given public key.

[cmars, 2012-12-12 02:23:32]

Adam, Thanks for reviewing today. I've uploaded a patch with your recommended changes. Best, Casey https://codereview.appspot.com/6927044/diff/2001/openpgp/keys.go File openpgp/keys.go (right): https://codereview.appspot.com/6927044/diff/2001/openpgp/keys.go#newcode209 openpgp/keys.go:209: // Skip unreadable badly-formatted keys On 2012/12/11 15:08:41, agl1 wrote: > comma after 'unreadable'. Done. https://codereview.appspot.com/6927044/diff/2001/openpgp/packet/opaque.go File openpgp/packet/opaque.go (right): https://codereview.appspot.com/6927044/diff/2001/openpgp/packet/opaque.go#new... openpgp/packet/opaque.go:9: // An opaque packet represents an OpenPGP packet as raw unparsed On 2012/12/11 15:08:41, agl1 wrote: > s/An opaque packet /OpaquePacket / Done. https://codereview.appspot.com/6927044/diff/2001/openpgp/packet/opaque.go#new... openpgp/packet/opaque.go:9: // An opaque packet represents an OpenPGP packet as raw unparsed On 2012/12/11 15:08:41, agl1 wrote: > comma after 'raw'. Done. https://codereview.appspot.com/6927044/diff/2001/openpgp/packet/opaque.go#new... openpgp/packet/opaque.go:9: // An opaque packet represents an OpenPGP packet as raw unparsed On 2012/12/11 15:08:41, agl1 wrote: > s/An opaque packet /OpaquePacket / Done. https://codereview.appspot.com/6927044/diff/2001/openpgp/packet/opaque.go#new... openpgp/packet/opaque.go:24: buf := bytes.NewBuffer([]byte{}) On 2012/12/11 15:08:41, agl1 wrote: > This looks like http://golang.org/pkg/io/ioutil/#ReadAll Done. https://codereview.appspot.com/6927044/diff/2001/openpgp/packet/opaque.go#new... openpgp/packet/opaque.go:30: // Serialize the packet to a writer in its original form including On 2012/12/11 15:08:41, agl1 wrote: > comma after 'form'. Done. https://codereview.appspot.com/6927044/diff/2001/openpgp/packet/opaque.go#new... openpgp/packet/opaque.go:40: // Attempt to parse the opaque contents into a structure supported On 2012/12/11 15:08:41, agl1 wrote: > // Parse attempts to parse... Done. https://codereview.appspot.com/6927044/diff/2001/openpgp/packet/opaque.go#new... openpgp/packet/opaque.go:42: // will still be an opaque packet. On 2012/12/11 15:08:41, agl1 wrote: > will be op. Done. https://codereview.appspot.com/6927044/diff/2001/openpgp/packet/opaque.go#new... openpgp/packet/opaque.go:44: hdr := bytes.NewBuffer([]byte{}) On 2012/12/11 15:08:41, agl1 wrote: > s/[]byte{}/nil/ Done. https://codereview.appspot.com/6927044/diff/2001/openpgp/packet/opaque.go#new... openpgp/packet/opaque.go:49: return Read(io.MultiReader(bytes.NewBuffer(hdr.Bytes()), bytes.NewBuffer(op.Contents))) On 2012/12/11 15:08:41, agl1 wrote: > I'm not sure why you're creating a new bytes.Buffer for the first argument here. Done. https://codereview.appspot.com/6927044/diff/2001/openpgp/packet/opaque.go#new... openpgp/packet/opaque.go:52: // Reads OpaquePackets from an io.Reader. On 2012/12/11 15:08:41, agl1 wrote: > OpaqueReader reads OpaquePackets from an io.Reader. Done. https://codereview.appspot.com/6927044/diff/2001/openpgp/packet/opaque.go#new... openpgp/packet/opaque.go:75: // Read a subpacket from the opaque contents. On 2012/12/11 15:08:41, agl1 wrote: > OpaqueSubpacket represents an unparsed OpenPGP subpacket, as found in signature > and user attribute packets. Done. https://codereview.appspot.com/6927044/diff/2001/openpgp/packet/opaque.go#new... openpgp/packet/opaque.go:83: func OpaqueSubpackets(contents []byte) (result []*OpaqueSubpacket, err error) { On 2012/12/11 15:08:41, agl1 wrote: > // OpaqueSubpackets ... Done. https://codereview.appspot.com/6927044/diff/2001/openpgp/packet/opaque.go#new... openpgp/packet/opaque.go:84: result = []*OpaqueSubpacket{} On 2012/12/11 15:08:41, agl1 wrote: > var result []*OpaqueSubpacket Done. https://codereview.appspot.com/6927044/diff/2001/openpgp/packet/opaque.go#new... openpgp/packet/opaque.go:86: subHeaderLen uint32 On 2012/12/11 15:08:41, agl1 wrote: > you're trying too hard to use named return values here. Done. https://codereview.appspot.com/6927044/diff/2001/openpgp/packet/packet.go File openpgp/packet/packet.go (right): https://codereview.appspot.com/6927044/diff/2001/openpgp/packet/packet.go#new... openpgp/packet/packet.go:345: backup := bytes.NewBuffer([]byte{}) On 2012/12/11 15:08:41, agl1 wrote: > the argument can just be nil, I think. Done. https://codereview.appspot.com/6927044/diff/2001/openpgp/packet/packet.go#new... openpgp/packet/packet.go:348: var retry bool On 2012/12/11 15:08:41, agl1 wrote: > rather than use this bool, I think you can just: > > switch err.(type) { > case errors.UnsupportedError, errors.UnkownPacketTypeError: > p = &OpaquePacket{...} > } > > although maybe that doesn't work for some reason. Done. https://codereview.appspot.com/6927044/diff/2001/openpgp/packet/public_key.go File openpgp/packet/public_key.go (right): https://codereview.appspot.com/6927044/diff/2001/openpgp/packet/public_key.go... openpgp/packet/public_key.go:416: // Return the bit length for the given public key. On 2012/12/11 15:08:41, agl1 wrote: > // BitLength returns the bit length for the given public key. Done.

[cmars, 2012-12-12 20:50:18]

Added copyright to new file opaque.go per contributor guidelines.

[agl1, 2012-12-15 19:30:18]

*** Submitted as https://code.google.com/p/go/source/detail?r=040ebe98531e&repo=crypto *** openpgp: improve parser resilience & flexibility, add PublicKey.BitLength() These are improvements I've made as necessary to develop Hockeypuck, an OpenPGP keyserver (https://launchpad.net/hockeypuck). The max armor line length was increased to 96 because some keyservers (SKS) will output armor with lines greater than 64 (the prior max). I've exposed packet.ReadEntity to support stream-parsing, useful for large SKS dump files. ReadKeyRing attempts to recover in the event of malformed data. Because many packet types are not yet supported, I added OpaquePacket to capture unsupported types for offline storage and later reparsing. R=agl CC=golang-dev https://codereview.appspot.com/6927044 Committer: Adam Langley <agl@golang.org>

[agl1, 2012-12-15 19:31:42]

I submitted this with trivial changes. One of which was to add a FIXME because the TeeReader sadly breaks the ability to process PGP files as a stream: Previously it was possible to feed a multi-gigabyte file through without using much memory but now the TeeReader will spool all of it. Probably not too hard to fix in the future so I've added a FIXME.

[remyoudompheng, 2013-07-21 19:50:55]

R=close