Issue 6555052: code review 6555052: crypto/x509: add DecryptBlock function for loading pass...
Descriptioncrypto/x509: add DecryptBlock function for loading password protected keys
Adds a DecryptBlock function which takes a password and a *pem.Block and
returns the decrypted DER bytes suitable for passing into other crypto/x509
functions.
Patch Set 1 #Patch Set 2 : diff -r 24d74bd32d81 https://code.google.com/p/go #Patch Set 3 : diff -r 24d74bd32d81 https://code.google.com/p/go #
Total comments: 31
Patch Set 4 : code review 6555052: crypto/x509: add DecryptBlock function for loading pass... #Patch Set 5 : code review 6555052: crypto/x509: add DecryptBlock function for loading pass... #Patch Set 6 : diff -r 44b9f2d5eb03 https://code.google.com/p/go #
Total comments: 9
Patch Set 7 : diff -r 96fde1b15506 https://code.google.com/p/go #Patch Set 8 : diff -r 96fde1b15506 https://code.google.com/p/go #
MessagesTotal messages: 12
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go
Sign in to reply to this message.
https://codereview.appspot.com/6555052/diff/4001/src/pkg/crypto/x509/block_de... File src/pkg/crypto/x509/block_decrypt.go (right): https://codereview.appspot.com/6555052/diff/4001/src/pkg/crypto/x509/block_de... src/pkg/crypto/x509/block_decrypt.go:1: package x509 Copyright missing. https://codereview.appspot.com/6555052/diff/4001/src/pkg/crypto/x509/block_de... src/pkg/crypto/x509/block_decrypt.go:1: package x509 block_decrypt.go is a rather generic name. pem_decrypt.go? https://codereview.appspot.com/6555052/diff/4001/src/pkg/crypto/x509/block_de... src/pkg/crypto/x509/block_decrypt.go:2: comment referencing some RFC or other standard where this is defined? https://codereview.appspot.com/6555052/diff/4001/src/pkg/crypto/x509/block_de... src/pkg/crypto/x509/block_decrypt.go:14: //cipherInfo represents how to create a block cipher for a decryption mode. space after "//" (here and throughout). https://codereview.appspot.com/6555052/diff/4001/src/pkg/crypto/x509/block_de... src/pkg/crypto/x509/block_decrypt.go:20: //deriveKey uses a key derivation function to stretch the password into a key Some reference to where this is defined? (If you just cribbed it from the OpenSSL source, that's fine, just mention that.) https://codereview.appspot.com/6555052/diff/4001/src/pkg/crypto/x509/block_de... src/pkg/crypto/x509/block_decrypt.go:22: func (c cipherInfo) deriveKey(pass []byte, salt []byte) []byte { (pass, salt []byte) https://codereview.appspot.com/6555052/diff/4001/src/pkg/crypto/x509/block_de... src/pkg/crypto/x509/block_decrypt.go:53: func DecryptBlock(b *pem.Block, pass []byte) ([]byte, error) { DecryptPEM or DecryptPEMBlock perhaps? https://codereview.appspot.com/6555052/diff/4001/src/pkg/crypto/x509/block_de... src/pkg/crypto/x509/block_decrypt.go:56: return nil, errors.New("x509: No DEK-Info header in block") lowercase letter after "x509: " in errors (here and below). https://codereview.appspot.com/6555052/diff/4001/src/pkg/crypto/x509/block_de... src/pkg/crypto/x509/block_decrypt.go:64: mode, hexiv := dek[:idx], dek[idx+1:] s/hexiv/hexIV/ https://codereview.appspot.com/6555052/diff/4001/src/pkg/crypto/x509/block_de... src/pkg/crypto/x509/block_decrypt.go:77: key := ciph.deriveKey(pass, iv[:8]) what if len(iv) < 8? https://codereview.appspot.com/6555052/diff/4001/src/pkg/crypto/x509/block_de... src/pkg/crypto/x509/block_decrypt.go:92: dlen := len(data) what if len(data) == 0? https://codereview.appspot.com/6555052/diff/4001/src/pkg/crypto/x509/block_de... src/pkg/crypto/x509/block_decrypt.go:94: if 1 > last || last > 8 { s/1 > last/last == 0/ (just a personal preference really, it takes slightly less time to understand.) https://codereview.appspot.com/6555052/diff/4001/src/pkg/crypto/x509/block_de... src/pkg/crypto/x509/block_decrypt.go:97: for _, val := range data[dlen-int(last):] { what if len(data) < last? https://codereview.appspot.com/6555052/diff/4001/src/pkg/crypto/x509/block_de... File src/pkg/crypto/x509/block_decrypt_test.go (right): https://codereview.appspot.com/6555052/diff/4001/src/pkg/crypto/x509/block_de... src/pkg/crypto/x509/block_decrypt_test.go:1: package x509 Copyright and filename. https://codereview.appspot.com/6555052/diff/4001/src/pkg/crypto/x509/block_de... src/pkg/crypto/x509/block_decrypt_test.go:28: pemdata []byte s/pemdata/pemData/ https://codereview.appspot.com/6555052/diff/4001/src/pkg/crypto/x509/block_de... src/pkg/crypto/x509/block_decrypt_test.go:33: pemdata: []byte(`-----BEGIN RSA PRIVATE KEY----- nit: the -----BEGIN could be on a new line. pem.Decode doesn't care.
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, agl@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
PTAL http://codereview.appspot.com/6555052/diff/4001/src/pkg/crypto/x509/block_dec... File src/pkg/crypto/x509/block_decrypt.go (right): http://codereview.appspot.com/6555052/diff/4001/src/pkg/crypto/x509/block_dec... src/pkg/crypto/x509/block_decrypt.go:1: package x509 On 2012/09/24 14:55:27, agl1 wrote: > Copyright missing. Done. http://codereview.appspot.com/6555052/diff/4001/src/pkg/crypto/x509/block_dec... src/pkg/crypto/x509/block_decrypt.go:2: On 2012/09/24 14:55:27, agl1 wrote: > comment referencing some RFC or other standard where this is defined? Done. http://codereview.appspot.com/6555052/diff/4001/src/pkg/crypto/x509/block_dec... src/pkg/crypto/x509/block_decrypt.go:14: //cipherInfo represents how to create a block cipher for a decryption mode. On 2012/09/24 14:55:27, agl1 wrote: > space after "//" (here and throughout). Done. http://codereview.appspot.com/6555052/diff/4001/src/pkg/crypto/x509/block_dec... src/pkg/crypto/x509/block_decrypt.go:20: //deriveKey uses a key derivation function to stretch the password into a key On 2012/09/24 14:55:27, agl1 wrote: > Some reference to where this is defined? (If you just cribbed it from the > OpenSSL source, that's fine, just mention that.) Done. http://codereview.appspot.com/6555052/diff/4001/src/pkg/crypto/x509/block_dec... src/pkg/crypto/x509/block_decrypt.go:22: func (c cipherInfo) deriveKey(pass []byte, salt []byte) []byte { On 2012/09/24 14:55:27, agl1 wrote: > (pass, salt []byte) Done. http://codereview.appspot.com/6555052/diff/4001/src/pkg/crypto/x509/block_dec... src/pkg/crypto/x509/block_decrypt.go:53: func DecryptBlock(b *pem.Block, pass []byte) ([]byte, error) { On 2012/09/24 14:55:27, agl1 wrote: > DecryptPEM or DecryptPEMBlock perhaps? Done. http://codereview.appspot.com/6555052/diff/4001/src/pkg/crypto/x509/block_dec... src/pkg/crypto/x509/block_decrypt.go:56: return nil, errors.New("x509: No DEK-Info header in block") On 2012/09/24 14:55:27, agl1 wrote: > lowercase letter after "x509: " in errors (here and below). Done. http://codereview.appspot.com/6555052/diff/4001/src/pkg/crypto/x509/block_dec... src/pkg/crypto/x509/block_decrypt.go:64: mode, hexiv := dek[:idx], dek[idx+1:] On 2012/09/24 14:55:27, agl1 wrote: > s/hexiv/hexIV/ Done. http://codereview.appspot.com/6555052/diff/4001/src/pkg/crypto/x509/block_dec... src/pkg/crypto/x509/block_decrypt.go:77: key := ciph.deriveKey(pass, iv[:8]) On 2012/09/24 14:55:27, agl1 wrote: > what if len(iv) < 8? Done. http://codereview.appspot.com/6555052/diff/4001/src/pkg/crypto/x509/block_dec... src/pkg/crypto/x509/block_decrypt.go:92: dlen := len(data) On 2012/09/24 14:55:27, agl1 wrote: > what if len(data) == 0? Done. http://codereview.appspot.com/6555052/diff/4001/src/pkg/crypto/x509/block_dec... src/pkg/crypto/x509/block_decrypt.go:94: if 1 > last || last > 8 { On 2012/09/24 14:55:27, agl1 wrote: > s/1 > last/last == 0/ > > (just a personal preference really, it takes slightly less time to understand.) Done. http://codereview.appspot.com/6555052/diff/4001/src/pkg/crypto/x509/block_dec... src/pkg/crypto/x509/block_decrypt.go:97: for _, val := range data[dlen-int(last):] { On 2012/09/24 14:55:27, agl1 wrote: > what if len(data) < last? Done. http://codereview.appspot.com/6555052/diff/4001/src/pkg/crypto/x509/block_dec... File src/pkg/crypto/x509/block_decrypt_test.go (right): http://codereview.appspot.com/6555052/diff/4001/src/pkg/crypto/x509/block_dec... src/pkg/crypto/x509/block_decrypt_test.go:1: package x509 On 2012/09/24 14:55:27, agl1 wrote: > Copyright and filename. Done. http://codereview.appspot.com/6555052/diff/4001/src/pkg/crypto/x509/block_dec... src/pkg/crypto/x509/block_decrypt_test.go:28: pemdata []byte On 2012/09/24 14:55:27, agl1 wrote: > s/pemdata/pemData/ Done. http://codereview.appspot.com/6555052/diff/4001/src/pkg/crypto/x509/block_dec... src/pkg/crypto/x509/block_decrypt_test.go:33: pemdata: []byte(`-----BEGIN RSA PRIVATE KEY----- On 2012/09/24 14:55:27, agl1 wrote: > nit: the -----BEGIN could be on a new line. pem.Decode doesn't care. Done.
Sign in to reply to this message.
https://codereview.appspot.com/6555052/diff/11001/src/pkg/crypto/x509/pem_dec... File src/pkg/crypto/x509/pem_decrypt.go (right): https://codereview.appspot.com/6555052/diff/11001/src/pkg/crypto/x509/pem_dec... src/pkg/crypto/x509/pem_decrypt.go:1: // Copyright 2011 The Go Authors. All rights reserved. 2012 https://codereview.appspot.com/6555052/diff/11001/src/pkg/crypto/x509/pem_dec... src/pkg/crypto/x509/pem_decrypt.go:8: // generate a key from the password was derived from looking at the OpenSSL s/from/by/ https://codereview.appspot.com/6555052/diff/11001/src/pkg/crypto/x509/pem_dec... src/pkg/crypto/x509/pem_decrypt.go:57: Do we need an IsEncryptedPEMBlock(b *pem.Block) bool function so that users of x509 know whether to prompt for a password? https://codereview.appspot.com/6555052/diff/11001/src/pkg/crypto/x509/pem_dec... src/pkg/crypto/x509/pem_decrypt.go:62: func DecryptPEMBlock(b *pem.Block, pass []byte) ([]byte, error) { Should it return an error if the PEM block is unencrypted, or should it simply return b.Bytes? I'm leaning towards the latter, although I'm not sure. https://codereview.appspot.com/6555052/diff/11001/src/pkg/crypto/x509/pem_dec... src/pkg/crypto/x509/pem_decrypt.go:106: return nil, errors.New("x509: invalid padding") If the password is incorrect then the padding will be invalid with probability ~= 1/256. I think that's probably accurate enough that we could do: var IncorrectPasswordError = errors.New("x509: decryption password incorrect") and return that rather than an "invalid padding" error.
Sign in to reply to this message.
On Mon, Sep 24, 2012 at 6:08 PM, <agl@golang.org> wrote: > If the password is incorrect then the padding will be invalid with > probability ~= 1/256. I think that's probably accurate enough that we > could do: Of course, I wanted the other side of that probability. So it should have read: "If the password is incorrect then the padding will be invalid with probability ~= 255/256." Cheers AGL
Sign in to reply to this message.
PTAL https://codereview.appspot.com/6555052/diff/11001/src/pkg/crypto/x509/pem_dec... File src/pkg/crypto/x509/pem_decrypt.go (right): https://codereview.appspot.com/6555052/diff/11001/src/pkg/crypto/x509/pem_dec... src/pkg/crypto/x509/pem_decrypt.go:57: On 2012/09/24 22:08:11, agl1 wrote: > Do we need an IsEncryptedPEMBlock(b *pem.Block) bool function so that users of > x509 know whether to prompt for a password? I figured most clients would attempt to load the file and if there was an error just assume it was password protected. The helper would probably just check the existence of the DEK-Info header which I'm not sure is worth the wait of the extra API. It's your call though. https://codereview.appspot.com/6555052/diff/11001/src/pkg/crypto/x509/pem_dec... src/pkg/crypto/x509/pem_decrypt.go:62: func DecryptPEMBlock(b *pem.Block, pass []byte) ([]byte, error) { On 2012/09/24 22:08:11, agl1 wrote: > Should it return an error if the PEM block is unencrypted, or should it simply > return b.Bytes? I'm leaning towards the latter, although I'm not sure. At first I had it return the bytes but then depending on if the block was encrypted you would either get a copy of the bytes or a slice referencing the blocks data. It felt ugly to have it return a copy for essentially no work done, so I left it as an error. https://codereview.appspot.com/6555052/diff/11001/src/pkg/crypto/x509/pem_dec... src/pkg/crypto/x509/pem_decrypt.go:106: return nil, errors.New("x509: invalid padding") On 2012/09/24 22:08:11, agl1 wrote: > If the password is incorrect then the padding will be invalid with probability > ~= 1/256. I think that's probably accurate enough that we could do: > > var IncorrectPasswordError = errors.New("x509: decryption password incorrect") > > and return that rather than an "invalid padding" error. Done. I left the first one as in invalid padding because having zero bytes would imply no padding which is invalid, not an incorrect password.
Sign in to reply to this message.
LGTM with that last function. https://codereview.appspot.com/6555052/diff/11001/src/pkg/crypto/x509/pem_dec... File src/pkg/crypto/x509/pem_decrypt.go (right): https://codereview.appspot.com/6555052/diff/11001/src/pkg/crypto/x509/pem_dec... src/pkg/crypto/x509/pem_decrypt.go:57: On 2012/09/24 22:25:49, zeebo wrote: > I figured most clients would attempt to load the file and if there was an error > just assume it was password protected. The helper would probably just check the > existence of the DEK-Info header which I'm not sure is worth the wait of the > extra API. It's your call though. I think the name "DEK-Info" is an internal aspect of the encryption and we shouldn't expect users of x509 to know about it. Since DecryptPEMBlock results in an error if the block isn't encrypted, then I think we need IsEncryptedPEMBlock.
Sign in to reply to this message.
PTAL
Sign in to reply to this message.
On Mon, Sep 24, 2012 at 6:36 PM, <jeff@spacemonkey.com> wrote: > PTAL (Sorry, just noticed that I dropped this. Will look at it this afternoon.) Cheers AGL
Sign in to reply to this message.
On 2012/09/27 13:44:23, agl1 wrote: > On Mon, Sep 24, 2012 at 6:36 PM, <mailto:jeff@spacemonkey.com> wrote: > > PTAL > > (Sorry, just noticed that I dropped this. Will look at it this afternoon.) > > > Cheers > > AGL Ping
Sign in to reply to this message.
*** Submitted as http://code.google.com/p/go/source/detail?r=8230013d1c40 *** crypto/x509: add DecryptBlock function for loading password protected keys Adds a DecryptBlock function which takes a password and a *pem.Block and returns the decrypted DER bytes suitable for passing into other crypto/x509 functions. R=golang-dev, agl, leterip CC=golang-dev http://codereview.appspot.com/6555052 Committer: Adam Langley <agl@golang.org>
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
