code review 5700087: crypto/x509: new home for root fetchers; build chains u...

crypto/x509: new home for root fetchers; build chains using Windows API

This moves the various CA root fetchers from crypto/tls into crypto/x509.

The move was brought about by issue 2997. Windows doesn't ship with all
its root certificates, but will instead download them as-needed when using
CryptoAPI for certificate verification.

This CL changes crypto/x509 to verify a certificate using the system root
CAs when VerifyOptions.RootCAs == nil. On Windows, this verification is
now implemented using Windows's CryptoAPI. All other root fetchers are
unchanged, and still use Go's own verification code.

The CL also fixes the hostname matching logic in crypto/tls/tls.go, in
order to be able to test whether hostname mismatches are honored by the
Windows verification code.

The move to crypto/x509 also allows other packages to use the OS-provided
root certificates, instead of hiding them inside the crypto/tls package.

Fixes issue 2997.

[mkrautz, 2012-02-28 00:10:35]

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

I'd like you to review this change to
https://code.google.com/p/go/

[mkrautz, 2012-02-28 00:12:03]

On 2012/02/28 00:10:35, mkrautz wrote:
> Hello mailto:agl@golang.org, mailto:brainman@gmail.com,
mailto:golang-dev@googlegroups.com (cc:
> mailto:golang-dev@googlegroups.com),
> 
> I'd like you to review this change to
> https://code.google.com/p/go/

This clashes with http://codereview.appspot.com/5700083/. I'll just sync this up
if that one hits the repo.

[agl1, 2012-02-28 00:51:16]

I'll have to leave the Windows specific stuff to someone else, but the x509
stuff LGTM. Although I would wonder whether it's not better to drop the bool in
CertPool and rather check whether it's the systemRoots in verify.go with

roots != nil && roots == systemRoots

[mkrautz, 2012-02-28 01:02:51]

On 2012/02/28 00:51:16, agl1 wrote:
> I'll have to leave the Windows specific stuff to someone else, but the x509
> stuff LGTM. Although I would wonder whether it's not better to drop the bool
in
> CertPool and rather check whether it's the systemRoots in verify.go with
> 
> roots != nil && roots == systemRoots

I'm not a fan either.

Since only Windows implements its own chain builder, I'd have to do something
like:

  roots != nil && roots == systemRoots && runtime.GOOS() == "windows"

Would you prefer that?

[agl1, 2012-02-28 01:08:29]

On 2012/02/28 01:02:51, mkrautz wrote:
>   roots != nil && roots == systemRoots && runtime.GOOS() == "windows"
> 
> Would you prefer that?

Ah yes, of course, we only want to trigger for Windows. If switching on GOOS()
is an established way to introduce Windows-only behaviour then I think that's
fine.

[brainman, 2012-02-28 05:17:07]

It is much cleaner this way.

Alex

http://codereview.appspot.com/5700087/diff/5001/src/pkg/crypto/x509/root_wind...
File src/pkg/crypto/x509/root_windows.go (right):

http://codereview.appspot.com/5700087/diff/5001/src/pkg/crypto/x509/root_wind...
src/pkg/crypto/x509/root_windows.go:23:
para.RequestedUsage.Usage.UsageIdentifier = nil
Shouldn't you restrict certificates you use to something like
szOID_PKIX_KP_SERVER_AUTH?

http://codereview.appspot.com/5700087/diff/5001/src/pkg/crypto/x509/root_wind...
src/pkg/crypto/x509/root_windows.go:69: }
Why aren't you using CertVerifyCertificateChainPolicy?

http://codereview.appspot.com/5700087/diff/5001/src/pkg/crypto/x509/root_wind...
src/pkg/crypto/x509/root_windows.go:85: if len(chain) !=
int(verifiedChain.NumElements) {
Why would these two be different?

http://codereview.appspot.com/5700087/diff/5001/src/pkg/syscall/syscall_windo...
File src/pkg/syscall/syscall_windows.go (right):

http://codereview.appspot.com/5700087/diff/5001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:188: //sys   CertFreeCertificateChain(ctx
*CertChainContext) (err error) = crypt32.CertFreeCertificateChain
CertFreeCertificateChain does not return anything:
s/ (err error)//

http://codereview.appspot.com/5700087/diff/5001/src/pkg/syscall/ztypes_window...
File src/pkg/syscall/ztypes_windows.go (right):

http://codereview.appspot.com/5700087/diff/5001/src/pkg/syscall/ztypes_window...
src/pkg/syscall/ztypes_windows.go:734: Chains                    
*CertSimpleChain
s/*CertSimpleChain/**CertSimpleChain/

http://codereview.appspot.com/5700087/diff/5001/src/pkg/syscall/ztypes_window...
src/pkg/syscall/ztypes_windows.go:736: LowerQualityChains        
*CertChainContext
s/*CertChainContext/**CertChainContext/

http://codereview.appspot.com/5700087/diff/5001/src/pkg/syscall/ztypes_window...
src/pkg/syscall/ztypes_windows.go:737: HasRevocationFreshnessTime bool
Is it safe to use bool here (and everywhere else) from alignment and value point
of view? Maybe just assume it is uint32.

http://codereview.appspot.com/5700087/diff/5001/src/pkg/syscall/ztypes_window...
src/pkg/syscall/ztypes_windows.go:745: Elements                  
*CertChainElement
s/*CertChainElement/**CertChainElement/

http://codereview.appspot.com/5700087/diff/5001/src/pkg/syscall/ztypes_window...
src/pkg/syscall/ztypes_windows.go:747: HasRevocationFreshnessTime bool
Same

http://codereview.appspot.com/5700087/diff/5001/src/pkg/syscall/ztypes_window...
src/pkg/syscall/ztypes_windows.go:766: HasFreshnessTime bool
Same

http://codereview.appspot.com/5700087/diff/5001/src/pkg/syscall/ztypes_window...
src/pkg/syscall/ztypes_windows.go:791: CheckRevocationFreshnessTime bool
Same

[rsc, 2012-02-28 17:20:16]

This is an enormous change to be making right now.
Is there a more limited change that can still fix this issue?

Russ

[mkrautz, 2012-02-28 17:22:23]

On 2012/02/28 17:20:16, rsc wrote:
> This is an enormous change to be making right now.
> Is there a more limited change that can still fix this issue?
> 
> Russ

This was the previous CL: http://codereview.appspot.com/5694078/

[agl1, 2012-02-28 17:24:44]

On Tue, Feb 28, 2012 at 9:20 AM, Russ Cox <rsc@golang.org> wrote:
> This is an enormous change to be making right now.
> Is there a more limited change that can still fix this issue?

I think this is probably the right thing to do but I think that it can
wait until after Go 1 if need be.


Cheers

AGL

[rsc, 2012-02-28 18:36:26]

Do the changes here modify or delete any APIs?  One thing that I liked
about the old system was that the nil CertPool (meaning system)
was not inspectable, so that if you wanted to, you could change
the implementation to let the system do full verification, because
you couldn't tell what the system's cert pool was anyway.
One thing this CL appears to do is make the system's cert pool
something that can be inspected, which either precludes calling
the OS's verification routines or allows the observation of inconsistent
behavior (a verification not justified by the pool).

Russ

[mkrautz, 2012-02-29 17:24:13]

On 2012/02/28 18:36:26, rsc wrote:
> Do the changes here modify or delete any APIs?  One thing that I liked
> about the old system was that the nil CertPool (meaning system)
> was not inspectable, so that if you wanted to, you could change
> the implementation to let the system do full verification, because
> you couldn't tell what the system's cert pool was anyway.
> One thing this CL appears to do is make the system's cert pool
> something that can be inspected, which either precludes calling
> the OS's verification routines or allows the observation of inconsistent
> behavior (a verification not justified by the pool).
> 
> Russ

Hi Russ,

The CL adds a new function to crypto/x509:

func SystemPools() *CertPool

It does not change or delete any existing part of
any APIs.

Package crypto/tls will still use the system pools
for verification like it always has, when RootCAs
in tls.Config is set to nil. It special cases
RootCAs == nil to mean x509.SystemPools(), which
is like that it did previously with varDefaultRoots
locally in its own package.

I agree that it is unfortunate that inconsistent
state can be observed. Specifically, I'm thinking
of the fact that the Subjects method of the system
cert pool will currently return an empty slice on
Windows.

I also do not like that it is possible to use add
certificates to the system pools via AddCert and
AppendCertsFromPEM, but those could be changed to
return an error if the pool is a system pool.

After thinking this over, I think we should just do
as crypto/tls does, and as you suggest Russ. We
should teach Certificate.Verify() to treat a
VerifyOptions struct with nil Intermediates and Roots
to mean 'verify against the system CAs'. That doesn't
export any magic CertPools that don't work like other
CertPools, including things like Subjects() method
that cannot be reliably implemented on Windows.

Sounds good?

[mkrautz, 2012-02-29 17:57:44]

On 2012/02/28 00:51:16, agl1 wrote:
> I'll have to leave the Windows specific stuff to someone else, but the x509
> stuff LGTM. Although I would wonder whether it's not better to drop the bool
in
> CertPool and rather check whether it's the systemRoots in verify.go with
> 
> roots != nil && roots == systemRoots

Hi AGL,

Am I correct in assuming that *Certificate.Verify()
is meant not to apply any specific policies to the
chains that it builds, and delegates that job to its
consumers?

I ask because that's what I'm currently doing on Windows:
Not requesting any specific usages, nor calling the
CertVerifyCertificateChainPolicy function.

Relatedly, I note that handshake_client.go, for
example, does not restrict the leaf certificate
received from the server to be
ExtKeyUsageServerAuth.

I have read the note about KeyUsage status flags in
verify.go, but I read that as only talking about
the basic KeyUsages and not ExtKeyUsage*. Perhaps
you could enlighten me. :-)

[agl1, 2012-03-02 16:22:27]

On 2012/02/29 17:57:44, mkrautz wrote:
> Am I correct in assuming that *Certificate.Verify()
> is meant not to apply any specific policies to the
> chains that it builds, and delegates that job to its
> consumers?

You mean X.509 policies identifiers? No, we don't currently do anything for
that. Not to say that we would never do so, but not yet.

> I have read the note about KeyUsage status flags in
> verify.go, but I read that as only talking about
> the basic KeyUsages and not ExtKeyUsage*. Perhaps
> you could enlighten me. :-)

I'm working through bits of the NIST PKITS when I can. Some extended key usage
code might appear in the future but we don't currently do anything with it.
That's likely to happen before X.509 policies though.


Cheers

AGL

[mkrautz, 2012-03-02 17:29:58]

On 2012/03/02 16:22:27, agl1 wrote:
> On 2012/02/29 17:57:44, mkrautz wrote:
> > Am I correct in assuming that *Certificate.Verify()
> > is meant not to apply any specific policies to the
> > chains that it builds, and delegates that job to its
> > consumers?
> 
> You mean X.509 policies identifiers? No, we don't currently do anything for
> that. Not to say that we would never do so, but not yet.

No, I simply meant policy as it's used in the MS CryptoAPI and Apple
Security.framework, where a SSL policy might match the leaf's CN to a hostname,
and ensure that ExtKeyUsageServerAuth or ExtKeyUsageClientAuth is set. Or a code
signing or authenticode policy might verify that ExtKeyUsageCodeSigning is set,
and that the roots are from a specific system-trusted store.

But I think your answer to the question below answered this question as well.
Since there's no extended key usage checking, the *Certificate.Verify() function
should only build chains for now, and let users of the API decide whether they
trust the returned chains for what they're doing. Is that correct?

> 
> > I have read the note about KeyUsage status flags in
> > verify.go, but I read that as only talking about
> > the basic KeyUsages and not ExtKeyUsage*. Perhaps
> > you could enlighten me. :-)
> 
> I'm working through bits of the NIST PKITS when I can. Some extended key usage
> code might appear in the future but we don't currently do anything with it.
> That's likely to happen before X.509 policies though.
> 
> 
> Cheers
> 
> AGL

[agl1, 2012-03-02 17:57:47]

On Fri, Mar 2, 2012 at 9:29 AM,  <krautz@gmail.com> wrote:
> No, I simply meant policy as it's used in the MS CryptoAPI and Apple
> Security.framework, where a SSL policy might match the leaf's CN to a
> hostname, and ensure that ExtKeyUsageServerAuth or ExtKeyUsageClientAuth
> is set. Or a code signing or authenticode policy might verify that
> ExtKeyUsageCodeSigning is set, and that the roots are from a specific
> system-trusted store.

Well, if the hostname is set in the VerifyOptions then we *do* match
that. (And that's pretty important!)

If the Windows implementation can't match a hostname without also
matching a key usage etc, then that's not the end of the world. In the
future, VerifyOptions can have a MustBeValidForTLSServer or something
that users could request for extra checking.


Cheers

AGL

[mkrautz, 2012-03-02 18:06:49]

On 2012/03/02 17:57:47, agl1 wrote:
> On Fri, Mar 2, 2012 at 9:29 AM,  <mailto:krautz@gmail.com> wrote:
> > No, I simply meant policy as it's used in the MS CryptoAPI and Apple
> > Security.framework, where a SSL policy might match the leaf's CN to a
> > hostname, and ensure that ExtKeyUsageServerAuth or ExtKeyUsageClientAuth
> > is set. Or a code signing or authenticode policy might verify that
> > ExtKeyUsageCodeSigning is set, and that the roots are from a specific
> > system-trusted store.
> 
> Well, if the hostname is set in the VerifyOptions then we *do* match
> that. (And that's pretty important!)
> 
> If the Windows implementation can't match a hostname without also
> matching a key usage etc, then that's not the end of the world. In the
> future, VerifyOptions can have a MustBeValidForTLSServer or something
> that users could request for extra checking.
> 
> 
> Cheers
> 
> AGL

Currently, we're still doing the hostname check in Go code:

The buildSystemChains method is inserted here:
http://codereview.appspot.com/5700087/patch/5001/6013

just below the current DNS check:
http://code.google.com/p/go/source/browse/src/pkg/crypto/x509/verify.go#142

But you're right, I didn't think of using the availability of a DNSName in
VerifyOptions to trigger a SSL policy check. That might be the sensible thing to
do.

[mkrautz, 2012-03-03 00:14:13]

PTAL.

I got rid of x509.SystemRoots() and instead use VerifyOptions.RootCAs == nil to
mean 'verify against system roots' (on Windows, 'verify using system APIs').

I also added a few tests and fixed a bug in tls.Dial. The logic for using the
hostname passed to tls.Dial as the tls.Config's ServerName was wrong.

http://codereview.appspot.com/5700087/diff/5001/src/pkg/crypto/x509/root_wind...
File src/pkg/crypto/x509/root_windows.go (right):

http://codereview.appspot.com/5700087/diff/5001/src/pkg/crypto/x509/root_wind...
src/pkg/crypto/x509/root_windows.go:23:
para.RequestedUsage.Usage.UsageIdentifier = nil
On 2012/02/28 05:17:07, brainman wrote:
> Shouldn't you restrict certificates you use to something like
> szOID_PKIX_KP_SERVER_AUTH?

Done, but only when opts.DNSName is set.

http://codereview.appspot.com/5700087/diff/5001/src/pkg/crypto/x509/root_wind...
src/pkg/crypto/x509/root_windows.go:69: }
On 2012/02/28 05:17:07, brainman wrote:
> Why aren't you using CertVerifyCertificateChainPolicy?

Done, for len(opts.DNSName) > 0.

http://codereview.appspot.com/5700087/diff/5001/src/pkg/crypto/x509/root_wind...
src/pkg/crypto/x509/root_windows.go:85: if len(chain) !=
int(verifiedChain.NumElements) {
On 2012/02/28 05:17:07, brainman wrote:
> Why would these two be different?

ParseCertificate could fail.

http://codereview.appspot.com/5700087/diff/5001/src/pkg/syscall/syscall_windo...
File src/pkg/syscall/syscall_windows.go (right):

http://codereview.appspot.com/5700087/diff/5001/src/pkg/syscall/syscall_windo...
src/pkg/syscall/syscall_windows.go:188: //sys   CertFreeCertificateChain(ctx
*CertChainContext) (err error) = crypt32.CertFreeCertificateChain
On 2012/02/28 05:17:07, brainman wrote:
> CertFreeCertificateChain does not return anything:
> s/ (err error)//

Done.

http://codereview.appspot.com/5700087/diff/5001/src/pkg/syscall/ztypes_window...
File src/pkg/syscall/ztypes_windows.go (right):

http://codereview.appspot.com/5700087/diff/5001/src/pkg/syscall/ztypes_window...
src/pkg/syscall/ztypes_windows.go:734: Chains                    
*CertSimpleChain
On 2012/02/28 05:17:07, brainman wrote:
> s/*CertSimpleChain/**CertSimpleChain/

Done.

http://codereview.appspot.com/5700087/diff/5001/src/pkg/syscall/ztypes_window...
src/pkg/syscall/ztypes_windows.go:736: LowerQualityChains        
*CertChainContext
On 2012/02/28 05:17:07, brainman wrote:
> s/*CertChainContext/**CertChainContext/

Done.

http://codereview.appspot.com/5700087/diff/5001/src/pkg/syscall/ztypes_window...
src/pkg/syscall/ztypes_windows.go:737: HasRevocationFreshnessTime bool
On 2012/02/28 05:17:07, brainman wrote:
> Is it safe to use bool here (and everywhere else) from alignment and value
point
> of view? Maybe just assume it is uint32.

Done.

http://codereview.appspot.com/5700087/diff/5001/src/pkg/syscall/ztypes_window...
src/pkg/syscall/ztypes_windows.go:745: Elements                  
*CertChainElement
On 2012/02/28 05:17:07, brainman wrote:
> s/*CertChainElement/**CertChainElement/

Done.

http://codereview.appspot.com/5700087/diff/5001/src/pkg/syscall/ztypes_window...
src/pkg/syscall/ztypes_windows.go:747: HasRevocationFreshnessTime bool
On 2012/02/28 05:17:07, brainman wrote:
> Same

Done.

http://codereview.appspot.com/5700087/diff/5001/src/pkg/syscall/ztypes_window...
src/pkg/syscall/ztypes_windows.go:766: HasFreshnessTime bool
On 2012/02/28 05:17:07, brainman wrote:
> Same

Done.

http://codereview.appspot.com/5700087/diff/5001/src/pkg/syscall/ztypes_window...
src/pkg/syscall/ztypes_windows.go:791: CheckRevocationFreshnessTime bool
On 2012/02/28 05:17:07, brainman wrote:
> Same

Done.

[mkrautz, 2012-03-03 12:19:27]

PTAL.

Stumbled upon a few issues in root_darwin.go and root_stub.go in the last one I
sent.

http://codereview.appspot.com/5700087/diff/5001/src/pkg/crypto/x509/root_wind...
File src/pkg/crypto/x509/root_windows.go (right):

http://codereview.appspot.com/5700087/diff/5001/src/pkg/crypto/x509/root_wind...
src/pkg/crypto/x509/root_windows.go:85: if len(chain) !=
int(verifiedChain.NumElements) {
On 2012/03/03 00:14:14, mkrautz wrote:
> On 2012/02/28 05:17:07, brainman wrote:
> > Why would these two be different?
> 
> ParseCertificate could fail.

I now return ParseCertificate's error instead of breaking out of the loop above.
This check is now gone.

[brainman, 2012-03-05 02:00:52]

LGTM

Leaving for others to make final call.

Alex

[rsc, 2012-03-05 21:31:49]

This seems okay to me, but please update the CL so that the
moved files are recorded as moved files; then we can see what
changed during the move.

Running 'hg addremove' should (after prompting you about
various other files) auto-detect these moves and update your
hg state.

Thanks.
Russ

[mkrautz, 2012-03-05 22:16:23]

PTAL.

On 2012/03/05 21:31:49, rsc wrote:
> This seems okay to me, but please update the CL so that the
> moved files are recorded as moved files; then we can see what
> changed during the move.
> 
> Running 'hg addremove' should (after prompting you about
> various other files) auto-detect these moves and update your
> hg state.

Done.

> 
> Thanks.
> Russ

[agl1, 2012-03-06 18:11:20]

I've sent out http://codereview.appspot.com/5753060/ to alter the deps to allow
this to land.

http://codereview.appspot.com/5700087/diff/26002/src/pkg/crypto/x509/verify_t...
File src/pkg/crypto/x509/verify_test.go (right):

http://codereview.appspot.com/5700087/diff/26002/src/pkg/crypto/x509/verify_t...
src/pkg/crypto/x509/verify_test.go:109: // can only return a single chain to us.
add " (for now)". CAPI is certainly capable of returning multiple chains itself.

http://codereview.appspot.com/5700087/diff/26002/src/pkg/crypto/x509/verify_t...
src/pkg/crypto/x509/verify_test.go:236: Intermediates: nil,
I think you need to fill in the intermediates. It's probably working for you
because CAPI will perform AIA chasing, but it'll make the tests flaky.

http://codereview.appspot.com/5700087/diff/26002/src/pkg/crypto/x509/verify_t...
src/pkg/crypto/x509/verify_test.go:266: NextOutputChain:
This is very copy paste from the above. Needs a TODO(agl) to clean it up at
least.

[rsc, 2012-03-06 18:19:10]

LGTM

The per-file diffs don't seem to be working but
'view raw patch set' seems okay.

Windows changed recently; you may need to hg sync, not sure.

[mkrautz, 2012-03-06 19:11:07]

PTAL

http://codereview.appspot.com/5700087/diff/26002/src/pkg/crypto/x509/verify_t...
File src/pkg/crypto/x509/verify_test.go (right):

http://codereview.appspot.com/5700087/diff/26002/src/pkg/crypto/x509/verify_t...
src/pkg/crypto/x509/verify_test.go:109: // can only return a single chain to us.
On 2012/03/06 18:11:21, agl1 wrote:
> add " (for now)". CAPI is certainly capable of returning multiple chains
itself.

Done.

A little sidenote:
As per the comments in root_windows.go, it seems to me like
CertGetCertificateChain can only build a single chain before returning. I'm not
suggesting we change it to use another function now (I realise this is already
quite a mouthful before Go 1), but I'm curious to know of a better way.

http://codereview.appspot.com/5700087/diff/26002/src/pkg/crypto/x509/verify_t...
src/pkg/crypto/x509/verify_test.go:236: Intermediates: nil,
On 2012/03/06 18:11:21, agl1 wrote:
> I think you need to fill in the intermediates. It's probably working for you
> because CAPI will perform AIA chasing, but it'll make the tests flaky.

This would be a no-op for the system verify test (because it only runs on
Windows). But I've done this anyway, since I refactored the test a bit.

The code in root_windows.go doesn't handle intermediates at all, and delegates
that to the CAPI call. I didn't see a way to pass a list of intermediates
initially, but I realise now that I should probably be passing an in-memory
store of intermediates as hAdditionalStore to CertGetCertificateChain.

http://codereview.appspot.com/5700087/diff/26002/src/pkg/crypto/x509/verify_t...
src/pkg/crypto/x509/verify_test.go:266: NextOutputChain:
On 2012/03/06 18:11:21, agl1 wrote:
> This is very copy paste from the above. Needs a TODO(agl) to clean it up at
> least.

Done. The test has been refactored to avoid the duplication.

[mikkel_krautz.dk, 2012-03-06 19:14:53]

On Tue, Mar 6, 2012 at 7:19 PM, Russ Cox <rsc@golang.org> wrote:
> LGTM
>
> The per-file diffs don't seem to be working but
> 'view raw patch set' seems okay.

Yeah, I made sure they were in the diff.  Looking at the per-file
diffs, they do have the git-style diff renames at the top of the file.
Is Rietveld supposed to mark them differently?

> Windows changed recently; you may need to hg sync, not sure.

[agl1, 2012-03-06 20:31:31]

http://codereview.appspot.com/5700087/diff/26002/src/pkg/crypto/x509/verify_t...
File src/pkg/crypto/x509/verify_test.go (right):

http://codereview.appspot.com/5700087/diff/26002/src/pkg/crypto/x509/verify_t...
src/pkg/crypto/x509/verify_test.go:236: Intermediates: nil,
On 2012/03/06 19:11:07, mkrautz wrote:
> The code in root_windows.go doesn't handle intermediates at all, and delegates
> that to the CAPI call. I didn't see a way to pass a list of intermediates
> initially, but I realise now that I should probably be passing an in-memory
> store of intermediates as hAdditionalStore to CertGetCertificateChain.

Ah, I'm afraid that passing the intermediates to the chain building is rather
important. Without it, the results will be flaky at best because CryptoAPI will
have to fetch them via HTTP. In many cases, there is no AIA pointer and so it
will simply fail.

See CreateOSCertChainForCert in
https://src.chromium.org/viewvc/chrome/trunk/src/net/base/x509_certificate_wi...

[rsc, 2012-03-06 20:32:52]

On Tue, Mar 6, 2012 at 14:14, Mikkel Krautz <mikkel@krautz.dk> wrote:
> Yeah, I made sure they were in the diff.  Looking at the per-file
> diffs, they do have the git-style diff renames at the top of the file.
> Is Rietveld supposed to mark them differently?

Probably hg upload again will fix it.  Rietveld can be flaky.

[mkrautz, 2012-03-07 00:10:25]

PTAL.

Now honors opts.Intermediates in the Windows system verification code.

[agl1, 2012-03-07 18:12:52]

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

crypto/x509: new home for root fetchers; build chains using Windows API

This moves the various CA root fetchers from crypto/tls into crypto/x509.

The move was brought about by issue 2997. Windows doesn't ship with all
its root certificates, but will instead download them as-needed when using
CryptoAPI for certificate verification.

This CL changes crypto/x509 to verify a certificate using the system root
CAs when VerifyOptions.RootCAs == nil. On Windows, this verification is
now implemented using Windows's CryptoAPI. All other root fetchers are
unchanged, and still use Go's own verification code.

The CL also fixes the hostname matching logic in crypto/tls/tls.go, in
order to be able to test whether hostname mismatches are honored by the
Windows verification code.

The move to crypto/x509 also allows other packages to use the OS-provided
root certificates, instead of hiding them inside the crypto/tls package.

Fixes issue 2997.

R=agl, golang-dev, alex.brainman, rsc, mikkel
CC=golang-dev
http://codereview.appspot.com/5700087

Committer: Adam Langley <agl@golang.org>