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: link Security.framework symbols without cgo #32604

Closed
FiloSottile opened this issue Jun 13, 2019 · 17 comments
Closed

crypto/x509: link Security.framework symbols without cgo #32604

FiloSottile opened this issue Jun 13, 2019 · 17 comments
Assignees
Milestone

Comments

@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Jun 13, 2019

Just like we link libSystem when CGO_ENABLED=0, we can probably do the same with Security.framework for obtaining the root CAs, and drop the horrible no-cgo fallback path that shells out to security. The latter is slow and makes some dangerous approximations due to not having access to the actual trust policies.

Suggested by @zx2c4.

@FiloSottile FiloSottile added this to the Go1.14 milestone Jun 13, 2019
@zx2c4
Copy link
Contributor

@zx2c4 zx2c4 commented Jun 13, 2019

and drop the horrible no-cgo fallback path that shells out to security.

And drop the cgo one too. There's not much of a strong reason for keeping cgo around when you can efficiently implement the same exact code in Go.

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 2, 2020

Change https://golang.org/cl/227037 mentions this issue: crypto/x509: use Security.framework without cgo for roots on macOS

@deitch
Copy link

@deitch deitch commented Apr 3, 2020

Wow that was quick @FiloSottile.

Is there an easy way to test it out locally? I’d like to be helpful.

@FiloSottile
Copy link
Member Author

@FiloSottile FiloSottile commented Apr 3, 2020

Not yet, it's just a stub making the linker parts work, I haven't ported the logic to Go yet. I'll ping this issue when it's ready to test, and I'll definitely appreciate testing in as many settings as possible.

@JohnStarich
Copy link

@JohnStarich JohnStarich commented Apr 21, 2020

Hey @FiloSottile, happy to help here too. We're seeing CGO-disabled CLI binaries are failing with cert verify errors, which I think could be fixed by this change.

@FiloSottile
Copy link
Member Author

@FiloSottile FiloSottile commented Apr 21, 2020

Could you test the CL on those machines?

GO111MODULE=on go get golang.org/dl/gotip@latest 
gotip download 227037    
GODEBUG=x509roots=1 gotip test crypto/x509 -v -run TestSystemRoots

You can also use gotip to build the binaries and see if it fixes it.

@JohnStarich
Copy link

@JohnStarich JohnStarich commented Apr 21, 2020

Thank you for the quick reply! I'll need to poke the affected users and have them run it. (My machine doesn't have a problem with CGO/no-CGO.)

@JohnStarich
Copy link

@JohnStarich JohnStarich commented Apr 21, 2020

Here's their results:

GODEBUG=x509roots=1 gotip test crypto/x509 -v -run TestSystemRoots
=== RUN   TestSystemRoots
crypto/x509: trust settings for CN=IBM VPN CA,O=International Business Machines Corporation,C=US: 1
crypto/x509: trust settings for CN=GeoTrust SSL CA,O=GeoTrust\, Inc.,C=US: 2
crypto/x509: trust settings for CN=IBM INTERNAL INTERMEDIATE CA,O=International Business Machines Corporation,C=US: 2
crypto/x509: trust settings for CN=DigiCert Global Root CA,OU=www.digicert.com,O=DigiCert Inc,C=US: 1
crypto/x509: trust settings for CN=com.apple.systemdefault,O=System Identity: 1
crypto/x509: trust settings for CN=com.apple.kerberos.kdc,O=System Identity: 1
crypto/x509: trust settings for CN=dlv-cert: 1
crypto/x509: trust settings for CN=IBM JSS Built-in Certificate Authority: 1
crypto/x509: trust settings for CN=IBM Internal Root CA,O=International Business Machines Corporation,C=US: 1
    TestSystemRoots: root_darwin_test.go:23: loadSystemRoots: 180.095636ms
crypto/x509: kSecTrustSettingsResultInvalid = 0
crypto/x509: kSecTrustSettingsResultTrustRoot = 1
crypto/x509: kSecTrustSettingsResultTrustAsRoot = 2
crypto/x509: kSecTrustSettingsResultDeny = 3
crypto/x509: kSecTrustSettingsResultUnspecified = 4
crypto/x509: com.apple.systemdefault returned 1
crypto/x509: com.apple.kerberos.kdc returned 1
crypto/x509: dlv-cert returned 1
crypto/x509: IBM JSS Built-in Certificate Authority returned 1
crypto/x509: IBM Internal Root CA returned 1
crypto/x509: IBM VPN CA returned 1
crypto/x509: GeoTrust SSL CA returned 2
crypto/x509: IBM INTERNAL INTERMEDIATE CA returned 2
crypto/x509: DigiCert Global Root CA returned 1
    TestSystemRoots: root_darwin_test.go:43: loadSystemRootsWithCgo: 165.60705ms
--- PASS: TestSystemRoots (0.35s)
PASS
ok  	crypto/x509	0.547s
And my machine's results, for comparison:
➜ testing GODEBUG=x509roots=1 gotip test crypto/x509 -v -run TestSystemRoots
=== RUN   TestSystemRoots
crypto/x509: trust settings for CN=IBM VPN CA,O=International Business Machines Corporation,C=US: 1
crypto/x509: trust settings for CN=*.ibm.com,O=International Business Machines Corporation,L=Armonk,ST=New York,C=US: 4
crypto/x509: trust settings for CN=*.pandora.com,OU=operations,O=Pandora Media\, Inc.,L=Oakland,ST=California,C=US: 4
crypto/x509: trust settings for CN=<redacted>: 4
crypto/x509: trust settings for CN=k8s-apiserver,L=CA,ST=San Francisco,C=US: 2
crypto/x509: trust settings for CN=k8s-apiserver,L=CA,ST=San Francisco,C=US: 4
crypto/x509: trust settings for CN=k8s-apiserver,L=CA,ST=San Francisco,C=US: 2
crypto/x509: trust settings for CN=IBM Internal Root CA,O=International Business Machines Corporation,C=US: 1
crypto/x509: trust settings for CN=IBM JSS Built-in Certificate Authority: 1
crypto/x509: trust settings for CN=IBM CA,O=International Business Machines Corporation,C=US: 1
crypto/x509: trust settings for CN=International Business Machines Corporation CA,O=International Business Machines Corporation: 1
    TestSystemRoots: root_darwin_test.go:23: loadSystemRoots: 184.048064ms
crypto/x509: kSecTrustSettingsResultInvalid = 0
crypto/x509: kSecTrustSettingsResultTrustRoot = 1
crypto/x509: kSecTrustSettingsResultTrustAsRoot = 2
crypto/x509: kSecTrustSettingsResultDeny = 3
crypto/x509: kSecTrustSettingsResultUnspecified = 4
crypto/x509: IBM Internal Root CA returned 1
crypto/x509: IBM JSS Built-in Certificate Authority returned 1
crypto/x509: IBM CA returned 1
crypto/x509: International Business Machines Corporation CA returned 1
crypto/x509: IBM VPN CA returned 1
crypto/x509: *.ibm.com returned 4
crypto/x509: *.pandora.com returned 4
crypto/x509: <redacted> returned 4
crypto/x509: k8s-apiserver returned 2
crypto/x509: k8s-apiserver returned 4
crypto/x509: k8s-apiserver returned 2
    TestSystemRoots: root_darwin_test.go:43: loadSystemRootsWithCgo: 133.480018ms
--- PASS: TestSystemRoots (0.32s)
PASS
ok      crypto/x509     0.690s
@JohnStarich
Copy link

@JohnStarich JohnStarich commented Apr 22, 2020

Follow up: the new binaries I built also work great for them. Seems like this is the fix we need! 🚀

@JohnStarich
Copy link

@JohnStarich JohnStarich commented May 1, 2020

Hey @FiloSottile, thanks again for your great work here. Is there anything I can do now to help nudge this one along?

@gopherbot gopherbot closed this in 6f52790 May 7, 2020
@JohnStarich
Copy link

@JohnStarich JohnStarich commented May 12, 2020

Glad to see it merged! Thanks again, @FiloSottile 🎉 🚀

@deitch
Copy link

@deitch deitch commented May 12, 2020

Great @FiloSottile ! Thank you.

@deitch
Copy link

@deitch deitch commented May 12, 2020

What go version(s) is this rolling into?

@odeke-em
Copy link
Member

@odeke-em odeke-em commented May 12, 2020

@JohnStarich
Copy link

@JohnStarich JohnStarich commented May 13, 2020

Oof. That’s a while off. Are there any compatibility concerns with cherry-picking into the next patch of 1.14?

@FiloSottile
Copy link
Member Author

@FiloSottile FiloSottile commented May 13, 2020

We only cherry-pick critical and safe fixes, and this is definitely too large and risky for a minor release, sorry.

@JohnStarich
Copy link

@JohnStarich JohnStarich commented May 14, 2020

No worries, I understand 👍

Thanks again!

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.

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