code review 6820114: crypto/x509: implement EncryptPEMBlock

crypto/x509: implement EncryptPEMBlock

Arbitrary decisions: order of the arguments and the
fact it takes a block-type argument (rather than
leaving to user to fill it in later); I'm happy whatever
colour we want to paint it.

We also change DecryptPEMBlock so that it won't
panic when the IV has the wrong size.

[rog, 2012-11-07 19:18:36]

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

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

[rog, 2012-11-08 13:41:47]

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

Please take another look.

[agl, 2012-11-08 14:04:02]

Moving today. Reviews will be delayed I'm afraid.
On Nov 8, 2012 8:41 AM, <rogpeppe@gmail.com> wrote:

> Hello agl@chromium.org (cc: golang-dev@googlegroups.com),
>
> Please take another look.
>
>
>
http://codereview.appspot.com/**6820114/<http://codereview.appspot.com/6820114/>
>

[agl1, 2012-11-09 15:38:14]

https://codereview.appspot.com/6820114/diff/6006/src/pkg/crypto/x509/pem_decr...
File src/pkg/crypto/x509/pem_decrypt.go (right):

https://codereview.appspot.com/6820114/diff/6006/src/pkg/crypto/x509/pem_decr...
src/pkg/crypto/x509/pem_decrypt.go:95: return nil, errors.New("x509: wrong IV
size")
s/wrong/incorrect/

https://codereview.appspot.com/6820114/diff/6006/src/pkg/crypto/x509/pem_decr...
src/pkg/crypto/x509/pem_decrypt.go:145: func EncryptPEMBlock(blockType string,
rand io.Reader, data, password []byte, alg string) (*pem.Block, error) {
rand is typically the first argument.

I also think that alg should be type and not a generic string. 

So

type PEMCipher int

const (
  PEMCipherDES PEMCipher = iota
  ...
  PEMCipherAES256
)

And I'd add PEMCipher as an element of rfc1423Algo and turn rfc1423Algos into a
slice, rather than a map. When we need to look one up, the code can just iterate
over the members.

https://codereview.appspot.com/6820114/diff/6006/src/pkg/crypto/x509/pem_decr...
src/pkg/crypto/x509/pem_decrypt.go:152: return nil, errors.New("cannot generate
IV: " + err.Error())
"x509: "

https://codereview.appspot.com/6820114/diff/6006/src/pkg/crypto/x509/pem_decr...
src/pkg/crypto/x509/pem_decrypt.go:154: key := ciph.deriveKey(password, iv[:8])
I'll take your word that the IV is truncated for this but a reference would be
nice if it's written down somewhere.

https://codereview.appspot.com/6820114/diff/6006/src/pkg/crypto/x509/pem_decr...
src/pkg/crypto/x509/pem_decrypt.go:159: dec := cipher.NewCBCEncrypter(block, iv)
surely the /en/crypter shouldn't be called 'dec'?

[rog, 2012-11-12 10:22:51]

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

Please take another look.

[agl1, 2012-11-12 14:00:11]

LGTM

[rog, 2012-11-12 15:26:04]

ha, more comments i forgot to publish

https://codereview.appspot.com/6820114/diff/6006/src/pkg/crypto/x509/pem_decr...
File src/pkg/crypto/x509/pem_decrypt.go (right):

https://codereview.appspot.com/6820114/diff/6006/src/pkg/crypto/x509/pem_decr...
src/pkg/crypto/x509/pem_decrypt.go:95: return nil, errors.New("x509: wrong IV
size")
On 2012/11/09 15:38:14, agl1 wrote:
> s/wrong/incorrect/

Done.

https://codereview.appspot.com/6820114/diff/6006/src/pkg/crypto/x509/pem_decr...
src/pkg/crypto/x509/pem_decrypt.go:145: func EncryptPEMBlock(blockType string,
rand io.Reader, data, password []byte, alg string) (*pem.Block, error) {
On 2012/11/09 15:38:14, agl1 wrote:
> rand is typically the first argument.
> 
> I also think that alg should be type and not a generic string. 
> 
> So
> 
> type PEMCipher int
> 
> const (
>   PEMCipherDES PEMCipher = iota
>   ...
>   PEMCipherAES256
> )
> 
> And I'd add PEMCipher as an element of rfc1423Algo and turn rfc1423Algos into
a
> slice, rather than a map. When we need to look one up, the code can just
iterate
> over the members.

Done. I considered keying the indexes of the slice by PEMCipher, but it seemed a
bit too precious.

I made the zero value an invalid cipher, but another possibility might be to
make the zero value be strong (e.g. PEMCipherAES256).

https://codereview.appspot.com/6820114/diff/6006/src/pkg/crypto/x509/pem_decr...
src/pkg/crypto/x509/pem_decrypt.go:152: return nil, errors.New("cannot generate
IV: " + err.Error())
On 2012/11/09 15:38:14, agl1 wrote:
> "x509: "

Done.

https://codereview.appspot.com/6820114/diff/6006/src/pkg/crypto/x509/pem_decr...
src/pkg/crypto/x509/pem_decrypt.go:154: key := ciph.deriveKey(password, iv[:8])
On 2012/11/09 15:38:14, agl1 wrote:
> I'll take your word that the IV is truncated for this but a reference would be
> nice if it's written down somewhere.

It matches the deriveKey call in DecryptPEMBlock. I've added a comment to that
effect.

https://codereview.appspot.com/6820114/diff/6006/src/pkg/crypto/x509/pem_decr...
src/pkg/crypto/x509/pem_decrypt.go:159: dec := cipher.NewCBCEncrypter(block, iv)
On 2012/11/09 15:38:14, agl1 wrote:
> surely the /en/crypter shouldn't be called 'dec'?

Done.

[rog, 2012-11-12 15:31:36]

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

crypto/x509: implement EncryptPEMBlock

Arbitrary decisions: order of the arguments and the
fact it takes a block-type argument (rather than
leaving to user to fill it in later); I'm happy whatever
colour we want to paint it.

We also change DecryptPEMBlock so that it won't
panic when the IV has the wrong size.

R=agl, agl
CC=golang-dev
http://codereview.appspot.com/6820114