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: honor OS X certificate trust settings #18141

Closed
quentinmit opened this Issue Dec 1, 2016 · 12 comments

Comments

Projects
None yet
7 participants
@quentinmit
Copy link
Contributor

quentinmit commented Dec 1, 2016

On Darwin, user's trust preferences for root certificates were not honored. If the user had a root certificate loaded in their Keychain that was explicitly not trusted, a Go program would still verify a connection using that root certificate.
Thanks to Xy Ziemba for identifying and reporting this issue.

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Dec 1, 2016

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

gopherbot pushed a commit that referenced this issue Dec 1, 2016

crypto/x509: read Darwin trust settings for root CAs
Darwin separately stores bits indicating whether a root certificate
should be trusted; this changes Go to read and use those when
initializing SystemCertPool.

Unfortunately, the trust API is very slow. To avoid a delay of up to
0.5s in initializing the system cert pool, we assume that
the trust settings found in kSecTrustSettingsDomainSystem will always
indicate trust. (That is, all root certs Apple distributes are trusted.)
This is not guaranteed by the API but is true in practice.

In the non-cgo codepath, we do not have that benefit, so we must check
the trust status of every certificate. This causes about 0.5s of delay
in initializing the SystemCertPool.

On OS X 10.11 and older, the "security" command requires a certificate
to be provided in a file and not on stdin, so the non-cgo codepath
creates temporary files for each certificate, further slowing initialization.

Updates #18141.

Change-Id: If681c514047afe5e1a68de6c9d40ceabbce54755
Reviewed-on: https://go-review.googlesource.com/33721
Run-TryBot: Quentin Smith <quentin@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Dec 1, 2016

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

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Dec 1, 2016

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

@broady broady added this to the Go1.7.4 milestone Dec 1, 2016

@bradfitz bradfitz added the Security label Dec 1, 2016

gopherbot pushed a commit that referenced this issue Dec 1, 2016

[release-branch.go1.7] crypto/x509: read Darwin trust settings for ro…
…ot CAs

Darwin separately stores bits indicating whether a root certificate
should be trusted; this changes Go to read and use those when
initializing SystemCertPool.

Unfortunately, the trust API is very slow. To avoid a delay of up to
0.5s in initializing the system cert pool, we assume that
the trust settings found in kSecTrustSettingsDomainSystem will always
indicate trust. (That is, all root certs Apple distributes are trusted.)
This is not guaranteed by the API but is true in practice.

In the non-cgo codepath, we do not have that benefit, so we must check
the trust status of every certificate. This causes about 0.5s of delay
in initializing the SystemCertPool.

On OS X 10.11 and older, the "security" command requires a certificate
to be provided in a file and not on stdin, so the non-cgo codepath
creates temporary files for each certificate, further slowing initialization.

Updates #18141.

Change-Id: If681c514047afe5e1a68de6c9d40ceabbce54755
Reviewed-on: https://go-review.googlesource.com/33721
Run-TryBot: Quentin Smith <quentin@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
Reviewed-on: https://go-review.googlesource.com/33727

gopherbot pushed a commit that referenced this issue Dec 1, 2016

[release-branch.go1.6] crypto/x509: read Darwin trust settings for ro…
…ot CAs

Darwin separately stores bits indicating whether a root certificate
should be trusted; this changes Go to read and use those when
initializing SystemCertPool.

Unfortunately, the trust API is very slow. To avoid a delay of up to
0.5s in initializing the system cert pool, we assume that
the trust settings found in kSecTrustSettingsDomainSystem will always
indicate trust. (That is, all root certs Apple distributes are trusted.)
This is not guaranteed by the API but is true in practice.

In the non-cgo codepath, we do not have that benefit, so we must check
the trust status of every certificate. This causes about 0.5s of delay
in initializing the SystemCertPool.

On OS X 10.11 and older, the "security" command requires a certificate
to be provided in a file and not on stdin, so the non-cgo codepath
creates temporary files for each certificate, further slowing initialization.

Updates #18141.

Change-Id: If681c514047afe5e1a68de6c9d40ceabbce54755
Reviewed-on: https://go-review.googlesource.com/33721
Run-TryBot: Quentin Smith <quentin@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
Reviewed-on: https://go-review.googlesource.com/33728

@broady broady closed this Dec 1, 2016

@bradfitz bradfitz changed the title embargoed security issue crypto/x509: honor OS X certificate trust settings Dec 1, 2016

@ransombriggs ransombriggs referenced this issue Dec 2, 2016

Merged

go1.7.4 #351

@micahlmartin

This comment has been minimized.

Copy link

micahlmartin commented Dec 14, 2016

I'm using terraform which has included this fix in it's latest version. The problem I'm having on my mac is that I'm getting net/http: TLS handshake timeout errors when trying to access any tls endpoints through this library. I'm on a corporate laptop that's pretty locked down so I'm sure it has something to do with it, but I'm not sure what I need to do to try and work around it. Any guidance would be much appreciated. Thanks!

@quentinmit

This comment has been minimized.

Copy link
Contributor

quentinmit commented Dec 14, 2016

@micahlmartin The changes for this bug should not be able to affect TLS handshake timeouts. Please open a new bug.

@mitchellh

This comment has been minimized.

Copy link

mitchellh commented Dec 16, 2016

This change appears to be causing intermittent "x509 certificate not trusted" errors for our non-cgo built Darwin binaries in Terraform: hashicorp/terraform#10779 (comment)

Starting with the release where we switched to 1.7.4, a number of users (not just the above issue) have reported that they intermittently get "x509 certificate not trusted" errors. As you can see from the issue, running it in a loop one user is seeing a 10% failure rate.

I'm not 100% sure this change is causing it, but in the version that this started happening, we had no changes related to the code where users are reporting this issue and we updated to Go 1.7.4.

Our builds do not use cgo since we cross-compile Darwin builds from Linux.

@bradfitz

This comment has been minimized.

Copy link
Member

bradfitz commented Dec 16, 2016

@mitchellh, can you verify whether https://go-review.googlesource.com/c/34389/ reduces or eliminates the flakiness?

As a speed optimization we tried piping into the macOS "security" tool, which works on Sierra, but it seems flaky, so the CL above passes it via a temp file always which is slower but seems to actually work. I hope that's what you're seeing.

@mitchellh

This comment has been minimized.

Copy link

mitchellh commented Dec 16, 2016

Hey @bradfitz I'll try to test that later today. In the mean time I can report that users with my identical-Terraform-commit build on top of Go 1.7.3 are not seeing any issues, so I'm more confident that this issue and your CL are on the path to success.

@bradfitz

This comment has been minimized.

Copy link
Member

bradfitz commented Dec 16, 2016

Unfortunately that CL by itself adds a 3.5 second delay to Go processes using TLS, so it needs some work.

@bradfitz

This comment has been minimized.

Copy link
Member

bradfitz commented Dec 16, 2016

Let's move discussion to #18203

@dharmapunk82

This comment has been minimized.

Copy link

dharmapunk82 commented Mar 6, 2017

I am seeing this in terraform 0.8.8, running on mac OS sierra 10.12.3, with go version go1.8 darwin/amd64 installed via homebrew. I don't see any difference when trying with terraform 0.8.8 downloaded from hasicorp directly as opposed to installed via homebrew.

Could we have a regression with go 1.8, since this seemed to start with go 1.7.4?

@bradfitz

This comment has been minimized.

Copy link
Member

bradfitz commented Mar 6, 2017

@dharmapunk82, we don't track comments on closed issues. If you have a reproducible problem, please file a new bug that can be tracked. I have questions about your statement, but I won't ask them here at risk of this turning into a discussion on a closed issue. Please file a new bug with details.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.