code review 5441052: crypto/tls: cleanup certificate load on windows

crypto/tls: cleanup certificate load on windows

- correct syscall.CertEnumCertificatesInStore so it returns error
- remove "reflect" dependency

[brainman, 2011-11-29 05:31:31]

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

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

[hector, 2011-11-29 09:38:59]

Please review http://codereview.appspot.com/5263045/ before making this change. 
The code originally parsed the certificate without copying it first but this was
deemed unsafe.

[agl1, 2011-11-29 14:25:45]

http://codereview.appspot.com/5441052/diff/3001/src/pkg/crypto/tls/root_windo...
File src/pkg/crypto/tls/root_windows.go (right):

http://codereview.appspot.com/5441052/diff/3001/src/pkg/crypto/tls/root_windo...
src/pkg/crypto/tls/root_windows.go:28: if c, err := x509.ParseCertificate(buf);
err == nil {
the x509.Certificate will contain slices of the input, so the copy would seem to
be important.

[brainman, 2011-11-30 00:57:17]

http://codereview.appspot.com/5441052/diff/3001/src/pkg/crypto/tls/root_windo...
File src/pkg/crypto/tls/root_windows.go (right):

http://codereview.appspot.com/5441052/diff/3001/src/pkg/crypto/tls/root_windo...
src/pkg/crypto/tls/root_windows.go:28: if c, err := x509.ParseCertificate(buf);
err == nil {
On 2011/11/29 14:25:45, agl1 wrote:
> ... copy would seem to
> be important.

Fair enough. I have changed my code. But I think the fact that
x509.ParseCertificate retains access to data buffer after it returns is
confusing. While, I admit, it should be safe to use in some scenarios, it could
be error prone in others (clients reuse certificate blob buffer).

I am not sure what is good solution here. Perhaps just copy array data at
ParseCertificate entry. Maybe document its behaviour somewhere, but then there
are other functions in x509 that act in a similar way. You are transfering
responsibility of managing shared data onto user of x509 package. Is that a good
decision?

[brainman, 2011-11-30 00:57:33]

Hello golang-dev@googlegroups.com, hectorchu@gmail.com, agl@golang.org (cc:
golang-dev@googlegroups.com, krautz@gmail.com),

Please take another look.

[rsc, 2011-11-30 20:16:25]

LGTM

I agree that the x509.ParseCertificate behavior is surprising.

[agl1, 2011-12-01 17:38:33]

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

crypto/tls: cleanup certificate load on windows

- correct syscall.CertEnumCertificatesInStore so it returns error
- remove "reflect" dependency

R=hectorchu, agl, rsc
CC=golang-dev, krautz
http://codereview.appspot.com/5441052

Committer: Adam Langley <agl@golang.org>