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