code review 12685044: openpgp: Implement compressed data packets & add suppor... (issue 12685044)

[mar...@toshnix.com]

Reviewers: golang-dev1,

Message:
Hello golan...@googlegroups.com,

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


Description:
openpgp: Implement compressed data packets & add support for compressing
data during symmetric encryption.

This patch implements the facilities needed to compress data before
encryption as allowed (and recommended) by RFC 4880. The new
functionality is then used to add support for compressing data during
symmetric encryption (openpgp.SymmetricallyEncrypt()).

For now, compression defaults to off. Also, only the ZIP and ZLIB
compression schemes are supported by this patch.

Resulting output tested/verified using GPG.

https://gist.github.com/marete/6189760 is a small program that can be
used to test that the output of various compression/encryption settings
can be read by GPG or other RFC 4880 programs.

Upon review, I will follow this patch with 2 others: a) Add support for
compression during public key encryption (openpgp.Encrypt()) b) Enable
compression by default (subject to the restrictions of the "Compression
Preferences" section in RFC 4880).

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

Affected files:
M openpgp/packet/compressed.go
M openpgp/packet/config.go
M openpgp/packet/packet.go
M openpgp/write.go

[a...@golang.org]

Mostly LGTM.

There's a lack of tests here. But I can imagine that the tests live in
the higher-level code that isn't in this change. If that's the case then
don't worry about duplicating them here.


https://codereview.appspot.com/12685044/diff/6001/openpgp/packet/compressed.go
File openpgp/packet/compressed.go (right):

https://codereview.appspot.com/12685044/diff/6001/openpgp/packet/compressed.go#newcode59
openpgp/packet/compressed.go:59: // header and the compressor. It's
Close() method ensures that both
It's -> Its

https://codereview.appspot.com/12685044/diff/6001/openpgp/packet/compressed.go#newcode77
openpgp/packet/compressed.go:77: err = cwc.sh.Close()
Can be just:

return cwc.sh.Close()

https://codereview.appspot.com/12685044/diff/6001/openpgp/packet/compressed.go#newcode107
openpgp/packet/compressed.go:107: s := strconv.FormatUint(uint64(algo),
10)
I think this should be strconv.Itoa(int(algo))

https://codereview.appspot.com/12685044/diff/6001/openpgp/packet/config.go
File openpgp/packet/config.go (right):

https://codereview.appspot.com/12685044/diff/6001/openpgp/packet/config.go#newcode43
openpgp/packet/config.go:43: DefaultCompressionConfig *CompressionConfig
While other members of Config are defaults and can be overridden by
lists of supported ciphers etc, I think this should just be called
"CompressionConfig", no? It's not the case that a public key can say
that it supports ZLIB, but only default compression.

https://codereview.appspot.com/12685044/diff/6001/openpgp/write.go
File openpgp/write.go (right):

https://codereview.appspot.com/12685044/diff/6001/openpgp/write.go#newcode125
openpgp/write.go:125: literaldata, err = packet.SerializeCompressed(w,
algo, level)
Some compression algorithms may have algorithm-specific options that
could be expressed in a CompressionConfig. But that would need
SerializeCompressed to take a *CompressionConfig rather than the level
directly.

So I think I would remove the CompressionLevel accessor from Config,
pass a *CompressionConfig into SerializeCompressed and have it with the
case that it's nil.

https://codereview.appspot.com/12685044/

[Brian Gitonga Marete]

Many thanks for the review. I will make the changes requested this evening.

Much appreciated.

Brian Gitonga Marete
CEO/CTO Toshnix Systems Limited
http://www.toshnix.com

[mar...@toshnix.com]

Hello golan...@googlegroups.com, a...@golang.org (cc:
golan...@googlegroups.com),

Please take another look.


https://codereview.appspot.com/12685044/

[mar...@toshnix.com]

[Brian Gitonga Marete]

This last change fixes comments.

Thanks.

--

Brian Gitonga Marete
CEO/CTO Toshnix Systems

http://toshnix.com

[v.h...@gmail.com]

Hello, is it now possible to use compression also during PKI Encryption?