code review 4551044: crypto/openpgp: add key generation support.

crypto/openpgp: add key generation support.

This change adds a function for generating new Entities and inchoate
support for reserialising Entities.

[agl1, 2011-05-17 17:07:33]

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

I'd like you to review this change to
https://go.googlecode.com/hg/

[bradfitz, 2011-05-17 17:57:37]

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

http://codereview.appspot.com/4551044/diff/4001/src/pkg/crypto/openpgp/keys.g...
src/pkg/crypto/openpgp/keys.go:307: func NewEntity(rand io.Reader,
currentTimeSecs int64, id string) (*Entity, os.Error) {
document the "id" parameter.

http://codereview.appspot.com/4551044/diff/4001/src/pkg/crypto/openpgp/packet...
File src/pkg/crypto/openpgp/packet/private_key.go (right):

http://codereview.appspot.com/4551044/diff/4001/src/pkg/crypto/openpgp/packet...
src/pkg/crypto/openpgp/packet/private_key.go:128: err =
error.InvalidArgumentError("unknown private key type")
maybe sprintf the #T type?  or at least say "not RSA".

http://codereview.appspot.com/4551044/diff/4001/src/pkg/crypto/openpgp/packet...
File src/pkg/crypto/openpgp/packet/signature.go (right):

http://codereview.appspot.com/4551044/diff/4001/src/pkg/crypto/openpgp/packet...
src/pkg/crypto/openpgp/packet/signature.go:446: func (sig *Signature)
SignUserId(id string, pub *PublicKey, priv *PrivateKey) (err os.Error) {
these two functions' docs aren't as clear as Sign() that they mutate/populate
"sig".  I suppose it's obvious, given that they only return an os.Error, but
it's a bit weird if you're only reading the docs.

http://codereview.appspot.com/4551044/diff/4001/src/pkg/crypto/openpgp/packet...
File src/pkg/crypto/openpgp/packet/userid.go (right):

http://codereview.appspot.com/4551044/diff/4001/src/pkg/crypto/openpgp/packet...
src/pkg/crypto/openpgp/packet/userid.go:23: func NewUserId(id string) *UserId {
This is the obvious constructor given that you get things in this form and
already have parseUserId(), but I think the more useful one in the context of
creating new Entities is a UserId constructor that takes the broken-out Name,
Comment, and Email and re-constructs the "Id" string with appropriate escaping
and omission of the comment if blank, etc, rather than making callers do that
themselves.

Maybe also add NewUser(name, comment, email string) *UserId?

http://codereview.appspot.com/4551044/diff/4001/src/pkg/crypto/openpgp/write_...
File src/pkg/crypto/openpgp/write_test.go (right):

http://codereview.appspot.com/4551044/diff/4001/src/pkg/crypto/openpgp/write_...
src/pkg/crypto/openpgp/write_test.go:55: e, err := NewEntity(rand.Reader,
time.Seconds(), "Test User (test) <test@example.com>")
I wonder if NewEntity should take a *UserId instead of a string so it could
benefit from the good UserId constructor.

[r, 2011-05-17 18:24:24]

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

http://codereview.appspot.com/4551044/diff/4001/src/pkg/crypto/openpgp/keys.g...
src/pkg/crypto/openpgp/keys.go:352: subkey.Sig.IssuerKeyId = &e.PrimaryKey.KeyId
would any of this code look better with composite literals?

http://codereview.appspot.com/4551044/diff/4001/src/pkg/crypto/openpgp/packet...
File src/pkg/crypto/openpgp/packet/public_key.go (right):

http://codereview.appspot.com/4551044/diff/4001/src/pkg/crypto/openpgp/packet...
src/pkg/crypto/openpgp/packet/public_key.go:276: // VerifyKeySignature returns
nil iff sig is a valid signature, make by this
s/make/made/?
in any case, i can't parse this.

http://codereview.appspot.com/4551044/diff/4001/src/pkg/crypto/openpgp/packet...
src/pkg/crypto/openpgp/packet/public_key.go:310: // VerifyUserIdSignature
returns nil iff sig is a valid signature, make by this
ditto

http://codereview.appspot.com/4551044/diff/4001/src/pkg/crypto/openpgp/packet...
File src/pkg/crypto/openpgp/packet/userid.go (right):

http://codereview.appspot.com/4551044/diff/4001/src/pkg/crypto/openpgp/packet...
src/pkg/crypto/openpgp/packet/userid.go:41: func (uid *UserId) Serialize(w
io.Writer) os.Error {
do these functions deserve doc comments?

http://codereview.appspot.com/4551044/diff/4001/src/pkg/crypto/openpgp/write_...
File src/pkg/crypto/openpgp/write_test.go (right):

http://codereview.appspot.com/4551044/diff/4001/src/pkg/crypto/openpgp/write_...
src/pkg/crypto/openpgp/write_test.go:85: t.Errorf("results differed")
worth printing them? or is it too messy?

[agl1, 2011-05-19 03:40:30]

Hello bradfitz@golang.org, r@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[agl1, 2011-05-19 03:42:55]

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

http://codereview.appspot.com/4551044/diff/4001/src/pkg/crypto/openpgp/keys.g...
src/pkg/crypto/openpgp/keys.go:307: func NewEntity(rand io.Reader,
currentTimeSecs int64, id string) (*Entity, os.Error) {
On 2011/05/17 17:57:37, bradfitz wrote:
> document the "id" parameter.

Done.

http://codereview.appspot.com/4551044/diff/4001/src/pkg/crypto/openpgp/keys.g...
src/pkg/crypto/openpgp/keys.go:352: subkey.Sig.IssuerKeyId = &e.PrimaryKey.KeyId
On 2011/05/17 18:24:24, r wrote:
> would any of this code look better with composite literals?

Yes, much. Thanks.

http://codereview.appspot.com/4551044/diff/4001/src/pkg/crypto/openpgp/packet...
File src/pkg/crypto/openpgp/packet/private_key.go (right):

http://codereview.appspot.com/4551044/diff/4001/src/pkg/crypto/openpgp/packet...
src/pkg/crypto/openpgp/packet/private_key.go:128: err =
error.InvalidArgumentError("unknown private key type")
On 2011/05/17 17:57:37, bradfitz wrote:
> maybe sprintf the #T type?  or at least say "not RSA".

We nearly don't need fmt in packet, so I'd like to not add any more calls. I've
rewritten the message instead.

http://codereview.appspot.com/4551044/diff/4001/src/pkg/crypto/openpgp/packet...
File src/pkg/crypto/openpgp/packet/public_key.go (right):

http://codereview.appspot.com/4551044/diff/4001/src/pkg/crypto/openpgp/packet...
src/pkg/crypto/openpgp/packet/public_key.go:276: // VerifyKeySignature returns
nil iff sig is a valid signature, make by this
On 2011/05/17 18:24:24, r wrote:
> s/make/made/?
> in any case, i can't parse this.

Done.

http://codereview.appspot.com/4551044/diff/4001/src/pkg/crypto/openpgp/packet...
src/pkg/crypto/openpgp/packet/public_key.go:310: // VerifyUserIdSignature
returns nil iff sig is a valid signature, make by this
On 2011/05/17 18:24:24, r wrote:
> ditto

Done.

http://codereview.appspot.com/4551044/diff/4001/src/pkg/crypto/openpgp/packet...
File src/pkg/crypto/openpgp/packet/signature.go (right):

http://codereview.appspot.com/4551044/diff/4001/src/pkg/crypto/openpgp/packet...
src/pkg/crypto/openpgp/packet/signature.go:446: func (sig *Signature)
SignUserId(id string, pub *PublicKey, priv *PrivateKey) (err os.Error) {
On 2011/05/17 17:57:37, bradfitz wrote:
> these two functions' docs aren't as clear as Sign() that they mutate/populate
> "sig".  I suppose it's obvious, given that they only return an os.Error, but
> it's a bit weird if you're only reading the docs.

Done.

http://codereview.appspot.com/4551044/diff/4001/src/pkg/crypto/openpgp/packet...
File src/pkg/crypto/openpgp/packet/userid.go (right):

http://codereview.appspot.com/4551044/diff/4001/src/pkg/crypto/openpgp/packet...
src/pkg/crypto/openpgp/packet/userid.go:23: func NewUserId(id string) *UserId {
I hesitated before because there's no convention for escaping the members of an
id. But see how you like the function now.

http://codereview.appspot.com/4551044/diff/4001/src/pkg/crypto/openpgp/packet...
src/pkg/crypto/openpgp/packet/userid.go:41: func (uid *UserId) Serialize(w
io.Writer) os.Error {
On 2011/05/17 18:24:24, r wrote:
> do these functions deserve doc comments?

Done.

http://codereview.appspot.com/4551044/diff/4001/src/pkg/crypto/openpgp/write_...
File src/pkg/crypto/openpgp/write_test.go (right):

http://codereview.appspot.com/4551044/diff/4001/src/pkg/crypto/openpgp/write_...
src/pkg/crypto/openpgp/write_test.go:55: e, err := NewEntity(rand.Reader,
time.Seconds(), "Test User (test) <test@example.com>")
On 2011/05/17 17:57:37, bradfitz wrote:
> I wonder if NewEntity should take a *UserId instead of a string so it could
> benefit from the good UserId constructor.

I'm trying to keep it so that you don't have to import packet unless you're
doing low level things. So I've added arguments for name, comment, email instead
and call NewUserId internally.

http://codereview.appspot.com/4551044/diff/4001/src/pkg/crypto/openpgp/write_...
src/pkg/crypto/openpgp/write_test.go:85: t.Errorf("results differed")
On 2011/05/17 18:24:24, r wrote:
> worth printing them? or is it too messy?

I think it's just going to be a huge mess I'm afraid.

[r, 2011-05-19 04:16:10]

LGTM for style

http://codereview.appspot.com/4551044/diff/9002/src/pkg/crypto/openpgp/packet...
File src/pkg/crypto/openpgp/packet/public_key.go (right):

http://codereview.appspot.com/4551044/diff/9002/src/pkg/crypto/openpgp/packet...
src/pkg/crypto/openpgp/packet/public_key.go:46: pk.IsSubkey = isSubkey
again, maybe composite literal?
but it's ok like this

http://codereview.appspot.com/4551044/diff/9002/src/pkg/crypto/openpgp/packet...
File src/pkg/crypto/openpgp/packet/userid.go (right):

http://codereview.appspot.com/4551044/diff/9002/src/pkg/crypto/openpgp/packet...
src/pkg/crypto/openpgp/packet/userid.go:26: case '(', ')', '<', '>':
i bet '\x00' is bad too. also i'm surprised control characters are allowed.

[bradfitzgoog, 2011-05-19 05:14:38]

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

http://codereview.appspot.com/4551044/diff/9002/src/pkg/crypto/openpgp/keys.g...
src/pkg/crypto/openpgp/keys.go:307: // which may be nil but must not contain any
of "()<>".
not technically nil for a string

http://codereview.appspot.com/4551044/diff/9002/src/pkg/crypto/openpgp/packet...
File src/pkg/crypto/openpgp/packet/signature.go (right):

http://codereview.appspot.com/4551044/diff/9002/src/pkg/crypto/openpgp/packet...
src/pkg/crypto/openpgp/packet/signature.go:447: func (sig *Signature)
SignUserId(id string, pub *PublicKey, priv *PrivateKey) (err os.Error) {
ditch the named err result param

http://codereview.appspot.com/4551044/diff/9002/src/pkg/crypto/openpgp/packet...
src/pkg/crypto/openpgp/packet/signature.go:448: h, err :=
userIdSignatureHash(id, pub, sig)
you're shadowing err here, no?

http://codereview.appspot.com/4551044/diff/9002/src/pkg/crypto/openpgp/packet...
src/pkg/crypto/openpgp/packet/signature.go:457: func (sig *Signature)
SignKey(pub *PublicKey, priv *PrivateKey) (err os.Error) {
ditch the named err result param

http://codereview.appspot.com/4551044/diff/9002/src/pkg/crypto/openpgp/packet...
src/pkg/crypto/openpgp/packet/signature.go:458: h, err :=
keySignatureHash(&priv.PublicKey, pub, sig)
you're shadowing err here, no?

[agl1, 2011-05-19 18:18:39]

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

http://codereview.appspot.com/4551044/diff/9002/src/pkg/crypto/openpgp/keys.g...
src/pkg/crypto/openpgp/keys.go:307: // which may be nil but must not contain any
of "()<>".
On 2011/05/19 05:14:38, bradfitzgoog wrote:
> not technically nil for a string

Done.

http://codereview.appspot.com/4551044/diff/9002/src/pkg/crypto/openpgp/packet...
File src/pkg/crypto/openpgp/packet/public_key.go (right):

http://codereview.appspot.com/4551044/diff/9002/src/pkg/crypto/openpgp/packet...
src/pkg/crypto/openpgp/packet/public_key.go:46: pk.IsSubkey = isSubkey
On 2011/05/19 04:16:10, r wrote:
> again, maybe composite literal?
> but it's ok like this

Done.

http://codereview.appspot.com/4551044/diff/9002/src/pkg/crypto/openpgp/packet...
File src/pkg/crypto/openpgp/packet/signature.go (right):

http://codereview.appspot.com/4551044/diff/9002/src/pkg/crypto/openpgp/packet...
src/pkg/crypto/openpgp/packet/signature.go:447: func (sig *Signature)
SignUserId(id string, pub *PublicKey, priv *PrivateKey) (err os.Error) {
On 2011/05/19 05:14:38, bradfitzgoog wrote:
> ditch the named err result param

Done.

http://codereview.appspot.com/4551044/diff/9002/src/pkg/crypto/openpgp/packet...
src/pkg/crypto/openpgp/packet/signature.go:448: h, err :=
userIdSignatureHash(id, pub, sig)
On 2011/05/19 05:14:38, bradfitzgoog wrote:
> you're shadowing err here, no?

I don't believe so.

http://codereview.appspot.com/4551044/diff/9002/src/pkg/crypto/openpgp/packet...
src/pkg/crypto/openpgp/packet/signature.go:457: func (sig *Signature)
SignKey(pub *PublicKey, priv *PrivateKey) (err os.Error) {
On 2011/05/19 05:14:38, bradfitzgoog wrote:
> ditch the named err result param

Done.

http://codereview.appspot.com/4551044/diff/9002/src/pkg/crypto/openpgp/packet...
File src/pkg/crypto/openpgp/packet/userid.go (right):

http://codereview.appspot.com/4551044/diff/9002/src/pkg/crypto/openpgp/packet...
src/pkg/crypto/openpgp/packet/userid.go:26: case '(', ')', '<', '>':
On 2011/05/19 04:16:10, r wrote:
> i bet '\x00' is bad too. also i'm surprised control characters are allowed.

Have removed NUL. As for the rest, since there's no standard, I'll err on the
side of being permissive.

[agl1, 2011-05-20 16:37:20]

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

crypto/openpgp: add key generation support.

This change adds a function for generating new Entities and inchoate
support for reserialising Entities.

R=bradfitz, r, bradfitz
CC=golang-dev
http://codereview.appspot.com/4551044

[bradfitz, 2011-05-20 17:01:22]

LGTM

Thanks again!

http://codereview.appspot.com/4551044/diff/9002/src/pkg/crypto/openpgp/packet...
File src/pkg/crypto/openpgp/packet/signature.go (right):

http://codereview.appspot.com/4551044/diff/9002/src/pkg/crypto/openpgp/packet...
src/pkg/crypto/openpgp/packet/signature.go:448: h, err :=
userIdSignatureHash(id, pub, sig)
On 2011/05/19 18:18:39, agl1 wrote:
> On 2011/05/19 05:14:38, bradfitzgoog wrote:
> > you're shadowing err here, no?
> 
> I don't believe so.

Wow, shows what I know.  Just tested it... you're right.  Good to know.