code review 5694078: crypto/tls: use windows's lazy root fetching

crypto/tls: use windows's lazy root fetching

Fixes issue 2997.

[mkrautz, 2012-02-25 16:52:48]

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

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

[agl1, 2012-02-26 21:18:09]

Thank you for doing the work but I'm afraid that I must admit to feeling that
this isn't right. It's also crypto/tls specific rather than being available to
any users of crypto/x509.

My feeling is that crypto/x509 can export a *CertPool on Windows meaning `the
system roots' and then crypto/x509.*Certificate.Verify can recognise this magic
and have CAPI build the chains directly. CAPI is generally very good code.

I think this reuses much of this CL and hopefully the result is cleaner and more
general.

[mkrautz, 2012-02-26 21:35:07]

On 2012/02/26 21:18:09, agl1 wrote:
> Thank you for doing the work but I'm afraid that I must admit to feeling that
> this isn't right. It's also crypto/tls specific rather than being available to
> any users of crypto/x509.
> 
> My feeling is that crypto/x509 can export a *CertPool on Windows meaning `the
> system roots' and then crypto/x509.*Certificate.Verify can recognise this
magic
> and have CAPI build the chains directly. CAPI is generally very good code.
> 
> I think this reuses much of this CL and hopefully the result is cleaner and
more
> general.

SGTM, but can we do this without adding exports to crypto/x509 (I suppose that
would break the API freeze)?

I suppose we could overload a nil CertPool to mean system roots for Windows, and
change it after Go 1. Any better suggestions?

[agl1, 2012-02-26 21:41:01]

On Sun, Feb 26, 2012 at 4:35 PM,  <krautz@gmail.com> wrote:
> I suppose we could overload a nil CertPool to mean system roots for
> Windows, and change it after Go 1. Any better suggestions?

Oh yes, absolutely. I think nil should mean system roots in
crypto/tls. Simply adding exports doesn't break an API, even if other
packages in the core use them, it's only when existing, external, code
would have to change that it's an issue.


Cheers

AGL

[mkrautz, 2012-02-26 21:55:10]

On 2012/02/26 21:41:01, agl1 wrote:
> On Sun, Feb 26, 2012 at 4:35 PM,  <mailto:krautz@gmail.com> wrote:
> > I suppose we could overload a nil CertPool to mean system roots for
> > Windows, and change it after Go 1. Any better suggestions?
> 
> Oh yes, absolutely. I think nil should mean system roots in
> crypto/tls. Simply adding exports doesn't break an API, even if other
> packages in the core use them, it's only when existing, external, code
> would have to change that it's an issue.

I actually meant in crypto/x509 -- to avoid adding an export. But if adding one
is fine, I'll do that.

Should I just move all root fetchers to crypto/x509 and export their pools via
x509.*SystemRootPool while I'm there?

[brainman, 2012-02-26 23:03:10]

Thank you for doing it. I am not expert in this area, so take my comments with a
grain of salt.

Alex

http://codereview.appspot.com/5694078/diff/2023/src/pkg/crypto/tls/handshake_...
File src/pkg/crypto/tls/handshake_client.go (left):

http://codereview.appspot.com/5694078/diff/2023/src/pkg/crypto/tls/handshake_...
src/pkg/crypto/tls/handshake_client.go:103: if !c.config.InsecureSkipVerify {
What about similar code in handshake_server.go?

http://codereview.appspot.com/5694078/diff/2023/src/pkg/crypto/tls/handshake_...
src/pkg/crypto/tls/handshake_client.go:117: c.verifiedChains, err =
certs[0].Verify(opts)
I think your logic is flawed here (for windows). You are checking against root
certificates that you have loaded at the start of your program. Well, if you
program runs for a while, these could change. Also, as we discovered
(http://code.google.com/p/go/issues/detail?id=2997), root certificate could be
missing - how do you know that one of these certificates is not been revoked (or
something) and should be invalid.

I would not even bother loading root certificates at any stage on windows, but
just use Windows api when needed. Just have verifyCert function that you call to
validate certificate. The function will load root certs and use
certs[0].Verify(opts) on unix, but on windows, it will call correspondent
Windows api.

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

http://codereview.appspot.com/5694078/diff/2023/src/pkg/crypto/tls/root_windo...
src/pkg/crypto/tls/root_windows.go:49: para.RequestedUsage.Usage.UsageIdentifier
= nil
I think you need to be more selective at what certificates you consider. See
http://stackoverflow.com/questions/7340504/whats-the-correct-way-to-verify-an...
or http://www.coastrd.com/c-schannel-smtp for examples.

http://codereview.appspot.com/5694078/diff/2023/src/pkg/crypto/tls/root_windo...
src/pkg/crypto/tls/root_windows.go:58: if chain.TrustStatus.ErrorStatus !=
syscall.CERT_TRUST_NO_ERROR {
Why aren't you using CertVerifyCertificateChainPolicy to decide on the outcome?
(http://stackoverflow.com/questions/7340504/whats-the-correct-way-to-verify-an...)

[mkrautz, 2012-02-26 23:18:21]

On 2012/02/26 23:03:10, brainman wrote:
> Thank you for doing it. I am not expert in this area, so take my comments with
a
> grain of salt.
> 
> Alex

Hi Alex,

I'll abandon this prepare a CL that moves this to crypto/x509 intsead.

> 
>
http://codereview.appspot.com/5694078/diff/2023/src/pkg/crypto/tls/handshake_...
> File src/pkg/crypto/tls/handshake_client.go (left):
> 
>
http://codereview.appspot.com/5694078/diff/2023/src/pkg/crypto/tls/handshake_...
> src/pkg/crypto/tls/handshake_client.go:103: if !c.config.InsecureSkipVerify {
> What about similar code in handshake_server.go?
> 
>
http://codereview.appspot.com/5694078/diff/2023/src/pkg/crypto/tls/handshake_...
> src/pkg/crypto/tls/handshake_client.go:117: c.verifiedChains, err =
> certs[0].Verify(opts)
> I think your logic is flawed here (for windows). You are checking against root
> certificates that you have loaded at the start of your program. Well, if you
> program runs for a while, these could change. Also, as we discovered
> (http://code.google.com/p/go/issues/detail?id=2997), root certificate could be
> missing - how do you know that one of these certificates is not been revoked
(or
> something) and should be invalid.
> 
> I would not even bother loading root certificates at any stage on windows, but
> just use Windows api when needed. Just have verifyCert function that you call
to
> validate certificate. The function will load root certs and use
> certs[0].Verify(opts) on unix, but on windows, it will call correspondent
> Windows api.

When we move this to crypto/x509, it will not load the root certs. It will just
use CAPI calls to fetch the chains.

In this CL, the only functionality that the call to CAPI gives us, is the
ability to force Windows to update its stores (if possible).

> 
>
http://codereview.appspot.com/5694078/diff/2023/src/pkg/crypto/tls/root_windo...
> File src/pkg/crypto/tls/root_windows.go (right):
> 
>
http://codereview.appspot.com/5694078/diff/2023/src/pkg/crypto/tls/root_windo...
> src/pkg/crypto/tls/root_windows.go:49:
para.RequestedUsage.Usage.UsageIdentifier
> = nil
> I think you need to be more selective at what certificates you consider. See
>
http://stackoverflow.com/questions/7340504/whats-the-correct-way-to-verify-an...
> or http://www.coastrd.com/c-schannel-smtp for examples.

In this CL, it's just attempting to update the root store, so any usage will do.
The Go code is still doing the verification.

>
http://codereview.appspot.com/5694078/diff/2023/src/pkg/crypto/tls/root_windo...
> src/pkg/crypto/tls/root_windows.go:58: if chain.TrustStatus.ErrorStatus !=
> syscall.CERT_TRUST_NO_ERROR {
> Why aren't you using CertVerifyCertificateChainPolicy to decide on the
outcome?
>
(http://stackoverflow.com/questions/7340504/whats-the-correct-way-to-verify-an...)

For this CL it's just a flag telling us whether a fetch might have occurred.

[mkrautz, 2012-02-26 23:19:35]

*** Abandoned ***