code review 3989052: crypto/openpgp: add package.

crypto/openpgp: add package

[agl1, 2011-01-30 21:17:20]

Hello bradfitzgo (cc: golang-dev@googlegroups.com),

I'd like you to review this change.

[rsc, 2011-01-30 22:21:03]

api notes

http://codereview.appspot.com/3989052/diff/4001/src/pkg/crypto/openpgp/canoni...
File src/pkg/crypto/openpgp/canonical_text.go (right):

http://codereview.appspot.com/3989052/diff/4001/src/pkg/crypto/openpgp/canoni...
src/pkg/crypto/openpgp/canonical_text.go:12: // NewCanonicalTextHash returns a
hash which wraps another while transforming
// NewCanonicalTextHash reformats text written to it into the canonical
// form and then applies the hash h.  See RFC ...

http://codereview.appspot.com/3989052/diff/4001/src/pkg/crypto/openpgp/keys.go
File src/pkg/crypto/openpgp/keys.go (right):

http://codereview.appspot.com/3989052/diff/4001/src/pkg/crypto/openpgp/keys.g...
src/pkg/crypto/openpgp/keys.go:16: // An Entity represents the componets of an
OpenPGP key. It consists of a
s/componets/components/
s/key. It consists of/key:/

http://codereview.appspot.com/3989052/diff/4001/src/pkg/crypto/openpgp/keys.g...
src/pkg/crypto/openpgp/keys.go:18: // claimed by that key and zero or more
subkeys, which may be encryption keys.
s/key/key,/

What is the map index for Identities?
If it is (as I would guess) the Name field,

    Identities map[string]*Identity  // indexed by Identity.Name

http://codereview.appspot.com/3989052/diff/4001/src/pkg/crypto/openpgp/keys.g...
src/pkg/crypto/openpgp/keys.go:34: // A SubKey is an additional public key in an
Entity. Subkeys can be used for
Since subkey is typically written as a single word, I'd spell this Subkey not
SubKey.

http://codereview.appspot.com/3989052/diff/4001/src/pkg/crypto/openpgp/keys.g...
src/pkg/crypto/openpgp/keys.go:56: // GetKeysById returns the set of keys that
have the given key id.
s/Get/get/

this is an exception to the usual capital letter policy.
Also get is almost always noise in Go.
This can just be keysById.

http://codereview.appspot.com/3989052/diff/4001/src/pkg/crypto/openpgp/keys.g...
src/pkg/crypto/openpgp/keys.go:83: func (kr *KeyRing) getAllDecryptionKeys()
(keys []Key) {
similarly allDecryptionKeys
or just decryptionKeys

http://codereview.appspot.com/3989052/diff/4001/src/pkg/crypto/openpgp/read.go
File src/pkg/crypto/openpgp/read.go (right):

http://codereview.appspot.com/3989052/diff/4001/src/pkg/crypto/openpgp/read.g...
src/pkg/crypto/openpgp/read.go:21: var SignatureType = "PGP SIGNATURE"
comment this

http://codereview.appspot.com/3989052/diff/4001/src/pkg/crypto/openpgp/read.g...
src/pkg/crypto/openpgp/read.go:272: // getHashForSignature returns a pair of
hashes that can be used to verify a
hashForSignature

[bradfitz, 2011-01-31 20:36:16]

http://codereview.appspot.com/3989052/diff/4001/src/pkg/crypto/openpgp/keys.go
File src/pkg/crypto/openpgp/keys.go (right):

http://codereview.appspot.com/3989052/diff/4001/src/pkg/crypto/openpgp/keys.g...
src/pkg/crypto/openpgp/keys.go:52: type KeyRing struct {
could you make this into an interface?  I'm going to need a bizarre KeyRing I
think.  Definitely not a static list of in-memory keys.

http://codereview.appspot.com/3989052/diff/4001/src/pkg/crypto/openpgp/read.go
File src/pkg/crypto/openpgp/read.go (right):

http://codereview.appspot.com/3989052/diff/4001/src/pkg/crypto/openpgp/read.g...
src/pkg/crypto/openpgp/read.go:48: Body                     io.Reader          
// the contents of the message.
Given the comment below, I wonder if it's better to name this "UntrustedBody" or
"UnverifiedBody".  It'd be uglier, but that's kinda the point?  It'd be hard to
mess up that way.

http://codereview.appspot.com/3989052/diff/4001/src/pkg/crypto/openpgp/read.g...
src/pkg/crypto/openpgp/read.go:70: // correct, the function will be called
again.
forever?

http://codereview.appspot.com/3989052/diff/4001/src/pkg/crypto/openpgp/read.g...
src/pkg/crypto/openpgp/read.go:71: type PromptFunction func(keys []Key,
symmetric bool) ([]byte, os.Error)
should this also take the try number then?  is there a way to signal from this
function's return value that retries should stop?

[agl1, 2011-01-31 21:01:38]

On Mon, Jan 31, 2011 at 3:36 PM,  <bradfitz@golang.org> wrote:
>
http://codereview.appspot.com/3989052/diff/4001/src/pkg/crypto/openpgp/keys.g...
> src/pkg/crypto/openpgp/keys.go:52: type KeyRing struct {
> could you make this into an interface?  I'm going to need a bizarre
> KeyRing I think.  Definitely not a static list of in-memory keys.

What are the limitations on your KeyRing? Is GetKeysById ok to
implement? GetAllDecryptionKeys is only needed when a recipient isn't
specified so you could skip that.

> Given the comment below, I wonder if it's better to name this
> "UntrustedBody" or "UnverifiedBody".  It'd be uglier, but that's kinda
> the point?  It'd be hard to mess up that way.

True and, in this case, hard to mess up is important. Any opinion on
whether the error should be on Close() or EOF?

>
http://codereview.appspot.com/3989052/diff/4001/src/pkg/crypto/openpgp/read.g...
> src/pkg/crypto/openpgp/read.go:70: // correct, the function will be
> called again.
> forever?

I assumed that it would be a closure, so it could keep its own count
of the number of times which it had been called. If the prompt
function always decrypts at least one private key then ReadMessage
will eventually figure out that none of the keys are going to work and
exit. If the message is passphrase encrypted then it will be called
forever to retry a different passphrase.

> should this also take the try number then?  is there a way to signal
> from this function's return value that retries should stop?

prompt can return an error value and that'll be passed up through ReadMessage.


AGL

[bradfitzgoog, 2011-01-31 21:43:54]

On Mon, Jan 31, 2011 at 1:01 PM, Adam Langley <agl@golang.org> wrote:

> On Mon, Jan 31, 2011 at 3:36 PM,  <bradfitz@golang.org> wrote:
> >
>
http://codereview.appspot.com/3989052/diff/4001/src/pkg/crypto/openpgp/keys.g...
> > src/pkg/crypto/openpgp/keys.go:52: type KeyRing struct {
> > could you make this into an interface?  I'm going to need a bizarre
> > KeyRing I think.  Definitely not a static list of in-memory keys.
>
> What are the limitations on your KeyRing? Is GetKeysById ok to
> implement? GetAllDecryptionKeys is only needed when a recipient isn't
> specified so you could skip that.
>

Yes, given a keyid I need to go do database/network requests to find it, if
possible.


> > Given the comment below, I wonder if it's better to name this
> > "UntrustedBody" or "UnverifiedBody".  It'd be uglier, but that's kinda
> > the point?  It'd be hard to mess up that way.
>
> True and, in this case, hard to mess up is important. Any opinion on
> whether the error should be on Close() or EOF?
>

I'd do it on EOF.  Harder to miss that way.  People are likely to ignore the
return value of Close().

(aside: I love that in Perl you can check whether you're being called in
scalar, list, or void context.  That means a function like Close() here
could panic() or do something explosive if the caller was in void context
and wasn't checking the return value... but I know that's not really Go
style anyway.)



>
> >
>
http://codereview.appspot.com/3989052/diff/4001/src/pkg/crypto/openpgp/read.g...
> > src/pkg/crypto/openpgp/read.go:70: // correct, the function will be
> > called again.
> > forever?
>
> I assumed that it would be a closure, so it could keep its own count
> of the number of times which it had been called. If the prompt
> function always decrypts at least one private key then ReadMessage
> will eventually figure out that none of the keys are going to work and
> exit. If the message is passphrase encrypted then it will be called
> forever to retry a different passphrase.
>

I think you need to document the expected return values of PromptFunction
then.  I misunderstood.


>
> > should this also take the try number then?  is there a way to signal
> > from this function's return value that retries should stop?
>
> prompt can return an error value and that'll be passed up through
> ReadMessage.
>
>
> AGL
>

[rsc, 2011-01-31 23:55:40]

If you're deciding between a signature verifier giving
the error on the final read instead of EOF or giving
the error on Close, please choose the former.

I'm pretty sure I have used that pattern elsewhere,
but I do not remember where.  In any event, people
checking return value from Read is much more common
than people checking return value from Close.

Russ

[rsc, 2011-01-31 23:58:48]

> I'm pretty sure I have used that pattern elsewhere,
> but I do not remember where.

remembered
src/pkg/compress/gzip/gunzip.go

russ

[agl1, 2011-02-14 17:37:07]

http://codereview.appspot.com/3989052/diff/4001/src/pkg/crypto/openpgp/canoni...
File src/pkg/crypto/openpgp/canonical_text.go (right):

http://codereview.appspot.com/3989052/diff/4001/src/pkg/crypto/openpgp/canoni...
src/pkg/crypto/openpgp/canonical_text.go:12: // NewCanonicalTextHash returns a
hash which wraps another while transforming
On 2011/01/30 22:21:03, rsc wrote:
> // NewCanonicalTextHash reformats text written to it into the canonical
> // form and then applies the hash h.  See RFC ...

Done.

http://codereview.appspot.com/3989052/diff/4001/src/pkg/crypto/openpgp/keys.go
File src/pkg/crypto/openpgp/keys.go (right):

http://codereview.appspot.com/3989052/diff/4001/src/pkg/crypto/openpgp/keys.g...
src/pkg/crypto/openpgp/keys.go:16: // An Entity represents the componets of an
OpenPGP key. It consists of a
On 2011/01/30 22:21:03, rsc wrote:
> s/componets/components/
> s/key. It consists of/key:/

Done.

http://codereview.appspot.com/3989052/diff/4001/src/pkg/crypto/openpgp/keys.g...
src/pkg/crypto/openpgp/keys.go:18: // claimed by that key and zero or more
subkeys, which may be encryption keys.
On 2011/01/30 22:21:03, rsc wrote:
> s/key/key,/
> 
> What is the map index for Identities?
> If it is (as I would guess) the Name field,
> 
>     Identities map[string]*Identity  // indexed by Identity.Name
> 

Done.

http://codereview.appspot.com/3989052/diff/4001/src/pkg/crypto/openpgp/keys.g...
src/pkg/crypto/openpgp/keys.go:34: // A SubKey is an additional public key in an
Entity. Subkeys can be used for
On 2011/01/30 22:21:03, rsc wrote:
> Since subkey is typically written as a single word, I'd spell this Subkey not
> SubKey.
> 

Done.

http://codereview.appspot.com/3989052/diff/4001/src/pkg/crypto/openpgp/keys.g...
src/pkg/crypto/openpgp/keys.go:56: // GetKeysById returns the set of keys that
have the given key id.
On 2011/01/30 22:21:03, rsc wrote:
> s/Get/get/
> 
> this is an exception to the usual capital letter policy.
> Also get is almost always noise in Go.
> This can just be keysById.

Done.

http://codereview.appspot.com/3989052/diff/4001/src/pkg/crypto/openpgp/keys.g...
src/pkg/crypto/openpgp/keys.go:83: func (kr *KeyRing) getAllDecryptionKeys()
(keys []Key) {
On 2011/01/30 22:21:03, rsc wrote:
> similarly allDecryptionKeys
> or just decryptionKeys

Done.

http://codereview.appspot.com/3989052/diff/4001/src/pkg/crypto/openpgp/read.go
File src/pkg/crypto/openpgp/read.go (right):

http://codereview.appspot.com/3989052/diff/4001/src/pkg/crypto/openpgp/read.g...
src/pkg/crypto/openpgp/read.go:21: var SignatureType = "PGP SIGNATURE"
On 2011/01/30 22:21:03, rsc wrote:
> comment this

Done.

http://codereview.appspot.com/3989052/diff/4001/src/pkg/crypto/openpgp/read.g...
src/pkg/crypto/openpgp/read.go:48: Body                     io.Reader          
// the contents of the message.
On 2011/01/31 20:36:16, bradfitzgo wrote:
> Given the comment below, I wonder if it's better to name this "UntrustedBody"
or
> "UnverifiedBody".  It'd be uglier, but that's kinda the point?  It'd be hard
to
> mess up that way.

Done.

http://codereview.appspot.com/3989052/diff/4001/src/pkg/crypto/openpgp/read.g...
src/pkg/crypto/openpgp/read.go:70: // correct, the function will be called
again.
On 2011/01/31 20:36:16, bradfitzgo wrote:
> forever?

Done.

http://codereview.appspot.com/3989052/diff/4001/src/pkg/crypto/openpgp/read.g...
src/pkg/crypto/openpgp/read.go:71: type PromptFunction func(keys []Key,
symmetric bool) ([]byte, os.Error)
On 2011/01/31 20:36:16, bradfitzgo wrote:
> should this also take the try number then?  is there a way to signal from this
> function's return value that retries should stop?

If the try number is important it can be a closure and keep track of that. Have
added a note about returning an error to break the loop.

http://codereview.appspot.com/3989052/diff/4001/src/pkg/crypto/openpgp/read.g...
src/pkg/crypto/openpgp/read.go:272: // getHashForSignature returns a pair of
hashes that can be used to verify a
On 2011/01/30 22:21:03, rsc wrote:
> hashForSignature

Done.

[bradfitz, 2011-02-23 16:34:30]

LGTM

I haven't ported Camlistore to this yet (which was my original plan before I
finished review), but seems fine.  I'll bug you later if I run into problems.

http://codereview.appspot.com/3989052/diff/11001/src/pkg/crypto/openpgp/keys.go
File src/pkg/crypto/openpgp/keys.go (right):

http://codereview.appspot.com/3989052/diff/11001/src/pkg/crypto/openpgp/keys....
src/pkg/crypto/openpgp/keys.go:29: Name          string // by convention, has
the form "Full Name (comment) <email@example.com>"
Pony request: methods to parse this as you have elsewhere.  Easy to add later,
though.

http://codereview.appspot.com/3989052/diff/11001/src/pkg/crypto/openpgp/keys....
src/pkg/crypto/openpgp/keys.go:99: // ReadArmoredKeyRing reads one or more
public/private keys from an armor keyring file.
s/ file// ?

http://codereview.appspot.com/3989052/diff/11001/src/pkg/crypto/openpgp/write.go
File src/pkg/crypto/openpgp/write.go (right):

http://codereview.appspot.com/3989052/diff/11001/src/pkg/crypto/openpgp/write...
src/pkg/crypto/openpgp/write.go:32: // DetachSignText signs message (after
canonicalising the line endings) with
izing  :)

http://codereview.appspot.com/3989052/diff/11001/src/pkg/crypto/openpgp/write...
src/pkg/crypto/openpgp/write.go:39: // ArmoredDetachSignText signs message
(after canonicalising the line endings)
izing  :)

[agl1, 2011-02-25 01:20:04]

*** Submitted as http://code.google.com/p/go/source/detail?r=9c3f4b89fcff ***

crypto/openpgp: add package

R=bradfitzgo
CC=golang-dev
http://codereview.appspot.com/3989052