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: Go 1.8's SystemCertPool() on Windows does not return all Windows root CAs #18609

Closed
mkrautz opened this issue Jan 11, 2017 · 12 comments
Closed

Comments

@mkrautz
Copy link
Contributor

@mkrautz mkrautz commented Jan 11, 2017

I believe that Go 1.8's implementation of SystemCertPool on Windows can give some surprising results.

The reason is that Windows doesn't ship with all of its root certificates installed. Instead, it downloads them on-demand.

(See the original implementation of systemVerify on Windows:a324a5a)

This means that there's a difference between what crypto/x509 will verify as OK on Windows with a default VerifyOptions (which uses the systemVerify() function - and will automatically fetch missing root CAs), and attempting to use the SystemCertPool() as the root store in VerifyOptions yourself.

I'm not sure what to do here. Maybe a note in the SystemCertPool docs about the Windows situation is sufficient?

@mkrautz mkrautz changed the title crypto/x509: SystemCertPool() on Windows does not return all Windows root CAs crypto/x509: Go 1.8's SystemCertPool() on Windows does not return all Windows root CAs Jan 11, 2017
@ALTree
Copy link
Member

@ALTree ALTree commented Jan 11, 2017

Reference issue is #16736 (crypto/x509: make SystemCertPool work on Windows), fixed in go1.8 by 05471e9.

cc @bradfitz @mattn

@mattn
Copy link
Member

@mattn mattn commented Jan 11, 2017

The reason is that Windows doesn't ship with all of its root certificates installed. Instead, it downloads them on-demand.

Yes, we know. It happend same on try-bot while review.

@mkrautz
Copy link
Contributor Author

@mkrautz mkrautz commented Jan 11, 2017

But surely that impacts the usability of SystemCertPool() quite drastically on Windows, to the point where it doesn't work like it does on any of the other platforms, no?

I think it's somewhat dangerous: previously, SystemCertPool() would return an error because the intended functionality couldn't be properly implemented on Windows. Now, it returns a subset of the Windows cert pool that that machine has visited before.

Programs that were aware of SystemCertPool returning an error on Windows might keep working.
But programs that weren't written with that in mind, or new programs that are written to expect SystemCertPool() to work the same way across all OSes will break in a non-deterministic manner (since it depends on which sites the machine running the program has visited before).

@mattn
Copy link
Member

@mattn mattn commented Jan 11, 2017

It is same that UNIX code use the files /etc/ssl/ca-bundle.pem. If the administrator doesn't update ca-bundle, it may be obsolete certificates.

https://github.com/golang/go/blob/master/src/crypto/x509/root_linux.go#L9-L13

@mkrautz
Copy link
Contributor Author

@mkrautz mkrautz commented Jan 11, 2017

Yes, the SystemCertPool() implementation may return obsolete certificates.

But that isn't the problem.

The problem is that SystemCertPool() isn't exhaustive. It doesn't contain certificates that the computer hasn't encountered/needed yet.

So that means the the behavior of something like:

// c is a *x509.Certificate.
opts := x509.VerifyOptions{
    Roots: x509.SystemCertPool(),
}
c.Verify(opts)

will return a different "truth" than

// c is a *x509.Certificate.
opts := x509.VerifyOptions{
    Roots: nil, // force use of systemVerify...
}
c.Verify(opts)

which to me is very unexpected.

@mattn
Copy link
Member

@mattn mattn commented Jan 11, 2017

It was kept to not change the behavior.

please see comments on https://go-review.googlesource.com/#/c/30578/1/src/crypto/x509/verify.go

@bradfitz bradfitz added this to the Go1.8 milestone Jan 11, 2017
@alexbrainman
Copy link
Member

@alexbrainman alexbrainman commented Jan 12, 2017

@mkrautz I agree that SystemCertPool on Windows could return misleading results. I do not see technical way to improve the situation - as you have explained above it just the way Windows works.

But (I am not security expert), I think other OSes would have similar problems. What about "certificate revocation lists"? Is SystemCertPool takes "certificate revocation lists" into account? Maybe something similar.

We should, probably, document SystemCertPool on Windows shortcomings.

Alex

@rsc
Copy link
Contributor

@rsc rsc commented Jan 17, 2017

It sounds like we should pull this from Go 1.8 and try again for Go 1.9.
We might return a CertPool that magically does the verify using the system store, but then there's a question of the interaction with AddCert and AppendCertsFromPEM.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Jan 17, 2017

Decision: we will revert this for Go 1.8.

We don't have time to get it right in a day or two. (Users want to append CAs to the system cert pool and verify with the union of the two; #13335)

@gopherbot
Copy link

@gopherbot gopherbot commented Jan 17, 2017

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

gopherbot pushed a commit that referenced this issue Jan 18, 2017
Updates #18609

Change-Id: I8306135660f52cf625bed4c7f53f632e527617de
Reviewed-on: https://go-review.googlesource.com/35265
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
Reviewed-by: Quentin Smith <quentin@golang.org>
@bradfitz bradfitz modified the milestones: Go1.9, Go1.8 Jan 18, 2017
@bradfitz bradfitz removed their assignment Jan 19, 2017
dmcgowan added a commit to dmcgowan/go-connections that referenced this issue Feb 3, 2017
The upstream change to allow access to the windows system
cert pool was reverted, reverting and updating messaging.
Maybe 1.9....golang/go#18609

Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
dmcgowan added a commit to dmcgowan/go-connections that referenced this issue Feb 3, 2017
The upstream change to allow access to the windows system
cert pool was reverted, reverting and updating messaging.
Maybe 1.9....golang/go#18609

Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
@jeffallen
Copy link
Contributor

@jeffallen jeffallen commented Feb 14, 2017

IMHO, the revert "fixed" this issue, and it is #16736 that should stay open, not this issue. I was just bitten by this, and having #16736 marked as closed confused me for a while.

@alexbrainman
Copy link
Member

@alexbrainman alexbrainman commented Feb 14, 2017

@jeffallen Done.

Alex

@golang golang locked and limited conversation to collaborators Feb 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
8 participants
You can’t perform that action at this time.