Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

crypto/x509: make SystemCertPool work on Windows? #16736

Open
bradfitz opened this issue Aug 16, 2016 · 22 comments
Open

crypto/x509: make SystemCertPool work on Windows? #16736

bradfitz opened this issue Aug 16, 2016 · 22 comments

Comments

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Aug 16, 2016

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." (a62ae9f, golang.org/cl/21293, #13335)

This bug is about fixing it.

/cc @alexbrainman

@bradfitz bradfitz added the OS-Windows label Aug 16, 2016
@bradfitz bradfitz added this to the Go1.8Maybe milestone Aug 16, 2016
@alexbrainman
Copy link
Member

@alexbrainman alexbrainman commented Aug 18, 2016

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
Copy link
Contributor Author

@bradfitz bradfitz commented Aug 22, 2016

Sounds plausible.

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

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 7, 2016

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

@quentinmit quentinmit added the NeedsFix label Oct 10, 2016
@gopherbot gopherbot closed this in 05471e9 Oct 17, 2016
mariash added a commit to concourse/fly that referenced this issue Nov 21, 2016
SystemCertPool is not supported on windows in go 1.7.
see golang/go#16736
Once 1.8 is released we can remove special condition and always append
to system cert pool.

[#133304007]

Signed-off-by: Maria Shaldibina <mshaldibina@pivotal.io>
@jeffallen
Copy link
Contributor

@jeffallen jeffallen commented Feb 14, 2017

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 alexbrainman modified the milestones: Go1.9, Go1.8Maybe Feb 14, 2017
@alexbrainman alexbrainman reopened this Feb 14, 2017
@alexbrainman
Copy link
Member

@alexbrainman alexbrainman commented Feb 14, 2017

@jeffallen Done.

Alex

@felixbecker
Copy link

@felixbecker felixbecker commented Mar 20, 2018

Hi, came from this issue #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.

falzm added a commit to exoscale/cli that referenced this issue Nov 6, 2019
This change introduces a workaround to a bug in the Microsoft Windows
support in Go (golang/go#16736). Some
heavily-chained certificates such as Exoscale SOS API's one are not
recognized as being signed by a trusted Certificate Authority.

Until the bug is fixed upstream, Windows users are required to use the
`--certs-file` flag with a path to a local file containing Exoscale SOS
API's certificate chain to the "exo sos" commands.

Fixes #189.
falzm added a commit to exoscale/cli that referenced this issue Nov 7, 2019
This change introduces a workaround to a bug in the Microsoft Windows
support in Go (golang/go#16736). Some
heavily-chained certificates such as Exoscale SOS API's one are not
recognized as being signed by a trusted Certificate Authority.

Until the bug is fixed upstream, Windows users are required to use the
`--certs-file` flag with a path to a local file containing Exoscale SOS
API's certificate chain to the "exo sos" commands.

Fixes #189.
falzm added a commit to exoscale/cli that referenced this issue Nov 7, 2019
This change introduces a workaround to a bug in the Microsoft Windows
support in Go (golang/go#16736). Some
heavily-chained certificates such as Exoscale SOS API's one are not
recognized as being signed by a trusted Certificate Authority.

Until the bug is fixed upstream, Windows users are required to use the
`--certs-file` flag with a path to a local file containing Exoscale SOS
API's certificate chain to the "exo sos" commands.

Fixes #189.
@rolandshoemaker
Copy link
Member

@rolandshoemaker rolandshoemaker commented Apr 1, 2020

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
Copy link

@elagergren-spideroak elagergren-spideroak commented Apr 1, 2020

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
Copy link
Contributor

@jeet-parekh jeet-parekh commented Apr 1, 2020

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.

@mxplusb
Copy link

@mxplusb mxplusb commented Apr 1, 2020

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
Copy link
Member

@rolandshoemaker rolandshoemaker commented Apr 1, 2020

@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
Copy link
Member

@rolandshoemaker rolandshoemaker commented Apr 1, 2020

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
Copy link

@tommed tommed commented Apr 1, 2020

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
Copy link
Contributor

@jeet-parekh jeet-parekh commented Apr 1, 2020

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.

@mxplusb
Copy link

@mxplusb mxplusb commented Apr 1, 2020

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
Copy link
Member

@rolandshoemaker rolandshoemaker commented Apr 1, 2020

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
Copy link
Member

@rasky rasky commented Sep 8, 2020

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 #16736 (comment) is doing something very very different compared to what happens on other operating systems?

@rolandshoemaker
Copy link
Member

@rolandshoemaker rolandshoemaker commented Sep 8, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

You can’t perform that action at this time.