crypto/x509: make SystemCertPool work on Windows?

[bradfitz]

https://golang.org/pkg/crypto/x509/#SystemCertPool doesn't work on Windows:

    func SystemCertPool() (*CertPool, error) {
        if runtime.GOOS == "windows" {
            return nil, errors.New("crypto/x509: system root pool is not available on Windows")
        }
        ....

I checked it in with the commit message "SystemCertPool returns an error on Windows. Maybe it's fixable later." (a62ae9f62fc, golang.org/cl/21293, #13335)

This bug is about fixing it.

/cc @alexbrainman

[alexbrainman]

I really don't know, I am not security expert. But I think you want to open LocalMachine\root (or maybe CurrentUser\root) certificate store, and read all certificates there with CertEnumCertificatesInStore or similar. What do you think?

Alex

[bradfitz]

Sounds plausible.

I don't think this requires a security expert as much as somebody who can read MSDN docs.

[gopherbot]

CL https://golang.org/cl/30578 mentions this issue.

[jeffallen]

Note: This change was rolled back in #18609. SystemCertPool on Windows on Go 1.8 still returns nil. @bradfitz Maybe you could re-open this and remove the go1.8maybe tag on it? Thanks.

[alexbrainman]

@jeffallen Done.

Alex

[felixbecker]

Hi, came from this issue https://github.com/golang/go/issues/18609 and try to understand what can help. Maybe as an look over the fence this is how dotnetcore address this (https://github.com/dotnet/corefx/tree/master/src/System.Security.Cryptography.X509Certificates/src/System/Security/Cryptography/X509Certificates). Just trying to get a better understanding what fails and what could help.

[sssilver]

Is there an estimate of when will this be fixed/released? Milestone is specified as Go1.11, which is due in July--is this accurate?

[alexbrainman]

Is there an estimate of when will this be fixed/released?

I don't believe anyone is working on this.

Alex

[gopherbot]

Change https://golang.org/cl/127577 mentions this issue: crypto/x509: make SystemCertPool work on Windows

[siennathesane]

For folks coming into this issue like me, you can work around it like this:

// +build windows

package main

import (
    "crypto/x509"
    "fmt"
    "syscall"
    "unsafe"
)

const (
    CRYPT_E_NOT_FOUND = 0x80092004
)

func main() {
    storeHandle, err := syscall.CertOpenSystemStore(0, syscall.StringToUTF16Ptr("Root"))
    if err != nil {
        fmt.Println(syscall.GetLastError())
    }

    var certs []*x509.Certificate
    var cert *syscall.CertContext
    for {
        cert, err = syscall.CertEnumCertificatesInStore(storeHandle, cert)
        if err != nil {
            if errno, ok := err.(syscall.Errno); ok {
                if errno == CRYPT_E_NOT_FOUND {
                    break
                }
            }
            fmt.Println(syscall.GetLastError())
        }
        if cert == nil {
            break
        }
        // Copy the buf, since ParseCertificate does not create its own copy.
        buf := (*[1 << 20]byte)(unsafe.Pointer(cert.EncodedCert))[:]
        buf2 := make([]byte, cert.Length)
        copy(buf2, buf)
        if c, err := x509.ParseCertificate(buf2); err == nil {
            certs = append(certs, c)
        }
    }
    fmt.Printf("%d", len(certs))
}

It grabbed everything default and custom root CAs I've installed:

PS C:\Users\MikeLloyd\.GoLand2019.2\config\scratches> go run .\scratch_1.go
85
PS C:\Users\MikeLloyd\.GoLand2019.2\config\scratches> (ls Cert:\CurrentUser\Root\).Count
85

I reviewed CL 127577 and it makes me really uncomfortable. I think the original recommendation of documenting expected behaviour (with docs references) and writing an idempotent test which doesn't break the try-bot should be enough for an MVP.

I asked @bketelsen to connect me with the Windows Certificate Store team out-of-band for advice on how to handle this situation, and then I should submit a CL soon which will address this.

[rolandshoemaker]

It would definitely be nice to solve this, many people seem to be taking the work around (run Verify twice, once with custom roots and once with no roots to get the systemVerify behavior), but that's less than ideal especially given SystemCertPool doesn't really document that this is necessary. The lack of documentation seems to be leading some people to take the rather worrying approach of just using InsecureSkipVerify on Windows...

Unfortunately given the design of the Windows certificate store there is no way to really accomplish this with CryptoAPI. Without adding magic behavior to Verify (i.e. adding a private flag or something to CertPool that tells Verify to fallback to systemVerify if no root is found in the actual pool) the only real solution is to take a similar approach to root_nocgo_darwin.go and shell out to certutil.

This is basically the approach suggested in CL 127577, but I think that CL took a somewhat more complex approach to the problem than is strictly necessary. A basic implementation would be something along the lines of:

1. Use certutil -syncWithWU to download the current full windows root trust list
2. Use certutil -verify to filter out any roots that were administratively distrusted
3. Use the original syscall.CertOpenSystemStore approach (from CL 30578) to retrieve the current system store, so that we get any administratively added roots

This would give a pool with the union of Windows trust + user trust (- user distrust).

cc @FiloSottile, thoughts?

[elagergren-spideroak]

Use certutil -syncWithWU to download the current full windows root trust list

I very strongly urge against any stdlib code accessing the network (modulo actual networking code).

In the general case, it's ineffective when the external network is down or spotty. It's sneaky and poor code and not something I'd expect out of an established project like Go.

Further, some programs are only allowed to access whitelisted URLs. Companies can get into real trouble if their application accesses non-whitelisted domains. It's one thing if the OS fetches certificates in the background as a normal part of using its system APIs (which I guess Windows does? I'm not sure). It's entirely different if it's from a shell command.

[jeet-parekh]

I agree, this issue is complex because you have to go over the network to fetch the certificates for windows.

Like @rolandshoemaker said, my CL is a bit complex because it collects certificates from all store locations and store names. After the certificates are downloaded, there's one more command to be run to extract the certificates from the downloaded dump (please correct me if I'm wrong here). I've also added some code to create a temp directory and delete the downloaded certificates after a SystemCertPool is populated. All of this adds to the complexity gradually.

Last I checked, syscall.CertOpenSystemStore wasn't collecting all the local certificates for me. If it does, then that would be a much cleaner approach than the one in my CL.

[siennathesane]

I think the most important aspect here that I want to double down on is what @elagergren-spideroak said:

I very strongly urge against any stdlib code accessing the network (modulo actual networking code).

Calling a system API that does not explicitly require network access should fundamentally be outside the scope of the implementation. It's up to individual developers to understand the workflow of using certificate stores, and I don't believe the implementation of the Windows API should do anything other than provide the simplest possible wrapper of CertOpenSystemStore.

It's sneaky and poor code and not something I'd expect out of an established project like Go.

I agree with this statement. All things being equal, this is a difficult and nuanced problem, but the solution should be the simplest and the most straightforward, bringing no opinions into it. Providing developers with the tools necessary to build what they need is more important than providing an opinionated solution, in this specific case.

It can be argued there is a use case where a developer is working with the certificate store, and needs a snapshot of the certificate store before it's updated, forcing a Windows Update as part of this API takes that opportunity away from the developer. Updating the certificate store should be something a developer has complete control over, putting in sneaky network calls takes that control away.

[rolandshoemaker]

@elagergren-spideroak

Use certutil -syncWithWU to download the current full windows root trust list

I very strongly urge against any stdlib code accessing the network (modulo actual networking code).

In the general case, it's ineffective when the external network is down or spotty. It's sneaky and poor code and not something I'd expect out of an established project like Go.

Further, some programs are only allowed to access whitelisted URLs. Companies can get into real trouble if their application accesses non-whitelisted domains. It's one thing if the OS fetches certificates in the background as a normal part of using its system APIs (which I guess Windows does? I'm not sure). It's entirely different if it's from a shell command.

While I won't argue that making a network call via a system utility isn't ideal or elegant, the choice is between that or not having a Windows system store at all, that's just how Window's CryptoAPI works unfortunately. If strongly documented I'm not that convinced that making retrieving the root store dependent on the network is necessarily bad, golang would end up mirroring the behavior of systemVerify, i.e. if you can't access the network and you don't have a root certificate in your local store already you won't be able to verify the certificate either way.

That said if there is strong opposition to adding this functionality then I think the real solution to this issue is to close it out with WONTFIX and to properly document in the SystemCertPool documentation that this won't work on Windows and to suggest the double verify workaround, since it's extremely unlikely CryptoAPI is going to change its behavior anytime soon.

@jeet-parekh

I agree, this issue is complex because you have to go over the network to fetch the certificates for windows.

Like @rolandshoemaker said, my CL is a bit complex because it collects certificates from all store locations and store names. After the certificates are downloaded, there's one more command to be run to extract the certificates from the downloaded dump (please correct me if I'm wrong here). I've also added some code to create a temp directory and delete the downloaded certificates after a SystemCertPool is populated. All of this adds to the complexity gradually.

Last I checked, syscall.CertOpenSystemStore wasn't collecting all the local certificates for me. If it does, then that would be a much cleaner approach than the one in my CL.

In your CL you are enumerating all the possible certificate stores, but only two of those are intended to handle root certificates that should be included in the pool, Root and CertificateAuthority. There is also no need to use powershell, or to convert the .cab files to .sst as certutil unpacks the roots itself. You also don't need to do anything with the disallowed cab since there is no crossover between the root trust list (it is intended to contain distrusted leaves, intermediates, and keys).

[rolandshoemaker]

Calling a system API that does not explicitly require network access should fundamentally be outside the scope of the implementation. It's up to individual developers to understand the workflow of using certificate stores, and I don't believe the implementation of the Windows API should do anything other than provide the simplest possible wrapper of CertOpenSystemStore.

The problem I see here is that SystemCertPool is designed as a generic API, if the Windows implementation ends up just being a basic wrapper around syscall.CertOpenSystemStore then it becomes the odd one out as it's behavior does not match at all what other systems provide. We can argue that users should know the ins and outs of the system they are developing on, but without strong documentation this becomes an API with very sharp corners that can become an easy foot gun.

[tommed]

Go isn't the first cross platform system to solve this design decision - can we inspire possible options from other cross-platform languages such as Ruby, Java, (dare I say it) PHP?

[jeet-parekh]

There is also no need to use powershell, or to convert the .cab files to .sst as certutil unpacks the roots itself.

I didn't know of that. It's been a long time since I last owned a Windows system. I do agree that making network calls doesn't feel ideal.

[siennathesane]

We can argue that users should know the ins and outs of the system they are developing on

I believe very strongly there is and should continue to be an expectation that developers should know the ins and outs of the APIs they are working with.

without strong documentation this becomes an API with very sharp corners that can become an easy foot gun.

I think adding winapi documentation references is the right path, but there should also be an expectation that developers understand the APIs they are working with. For low-level, highly nuanced APIs like this, there shouldn't be any protections for the developer, if they are making these calls they should be reading the winapi documentation on how to use it and this behaviour should only be a very thin wrapper for the winapi.

[rolandshoemaker]

I think adding winapi documentation references is the right path, but there should also be an expectation that developers understand the APIs they are working with. For low-level, highly nuanced APIs like this, there shouldn't be any protections for the developer, if they are making these calls they should be reading the winapi documentation on how to use it and this behaviour should only be a very thin wrapper for the winapi.

But this isn't really a very low-level nuanced API, whereas syscall.CertOpenSystemStore certainly is. On (basically) every other system SystemCertPool does a very simple, straight forward thing, it provides a list of certificates that the system trusts. If a thin wrapper around syscall.CertOpenSystemStore is added for Windows then that changes and it does become a nuanced API, because it'll do one thing one most systems and another very, very different thing on Windows.

[rasky]

If a thin wrapper around syscall.CertOpenSystemStore is added for Windows then that changes and it does become a nuanced API, because it'll do one thing one most systems and another very, very different thing on Windows.

Can you please clarify this? How the snippet in https://github.com/golang/go/issues/16736#issuecomment-540373689 is doing something very very different compared to what happens on other operating systems?

[rolandshoemaker]

Can you please clarify this? How the snippet in #16736 (comment) is doing something very very different compared to what happens on other operating systems?

Windows root store is dynamically loaded. So unlike on Linux/macOS, CertEnumCertificatesInStore doesn't return every root you trust, it returns the roots you trust which you've already encountered. This means that if you haven't yet used Windows' CryptoAPI to verify a certificate chain including root X then CertEnumCertificatesInStore won't return root X, even if it is actually trusted. Using it as a drop in for SystemCertPool means the user would typically get only a subset of trusted roots.

[docsmooth]

Edit as upon further reading I see that the below is exactly what @mxplusb is doing in the workaround @nikkictl merged 2 weeks before my post.

IIRC, instead of calling certutil to update the internet, and instead of calling CertEnumCertificatesInStore() (which as @rolandshoemaker pointed out, is dynamic), can't you just enumerate the 2 stores yourself? Here's PowerShell I use to do that all the time:`$certlist=@() $certStore = "Root" foreach ($certRootStore in @("LocalMachine", "CurrentUser")) { $store = new-object system.security.cryptography.x509certificates.x509Store $certStore, $certRootStore $store.Open('ReadOnly') gci cert:\$certRootStore\$certStore | foreach-object { $certlist+=$_ } }

return $certlist`

If you're testing that in PoSH (Linux pwsh 6 doesn't have a Cert drive, fyi), instead of "return $certlist" do: $certlist | foreach-object { write-host $_.Thumbprint }

You'll see a HUGE number of thumbprints printed, one for each cert in the store (check by running "certlm.msc" from the Run window to and count the output thumbprints from the local store.

[danielorbach]

I have encountered the lack of support for this function on Windows, and would like to help resolve it :)

[siennathesane]

I have encountered the lack of support for this function on Windows, and would like to help resolve it :)

@danielorbach, try this: https://github.com/golang/go/issues/16736#issuecomment-540373689

[gopherbot]

Change https://golang.org/cl/353589 mentions this issue: crypto/x509: verification with system and custom roots