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: darwin only loads system.root keychain should also load system keychain #16532

Closed
jostockley opened this issue Jul 28, 2016 · 27 comments
Closed

Comments

@jostockley
Copy link

@jostockley jostockley commented Jul 28, 2016

Please answer these questions before submitting your issue. Thanks!

  1. What version of Go are you using (go version)?
    1.6
  2. What operating system and processor architecture are you using (go env)?
    amd64 darwin El Capitan
  3. What did you do?
    I am trying to run minikube start on a mac running el capitan. I am inside my corporate network and they have a TLS man-in-the-middle box between the internal network and the internet so that when a TLS connection is made to an internet site, it generates an SSL certificate signed by the corporate root CA. This is installed in my Mac in the system keychain since it is not possible to install trusted CA root certs in the system.root keychain. However, in src/crypto/x509/root_darwin.go I see this:
func execSecurityRoots() (*CertPool, error) {
        cmd := exec.Command("/usr/bin/security", "find-certificate", "-a", "-p", "/System/Library/Keychains/SystemRootCertificates.keychain")
        data, err := cmd.Output()
        if err != nil {
                return nil, err
        }

        roots := NewCertPool()
        roots.AppendCertsFromPEM(data)
        return roots, nil
}

I believe the code should also load certificates from /System/Library/Keychains/SystemCACertificates.keychain

so as to pick up any user installed root certificates.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Jul 29, 2016

I suspect this is a dup of #14514 which was fixed for Go 1.7. Can you verify?

@jostockley
Copy link
Author

@jostockley jostockley commented Jul 29, 2016

I don't see any difference using 1.7rc3, here's my example:

package main

import (
    "log"
    "net/http"
)

func main() {
    _, err := http.Get("https://storage.googleapis.com/minikube/releases.json")
    if err != nil {
        log.Fatal(err)
    }
}

The certificate sent by googleapis is intercepted by our corporate mitm server and a substitute certificate is generated and signed by our internal enterprise intermediate CA SSL which is in turn signed by the internal enterprise CA Root. The program fails with this error:

2016/07/29 11:49:35 Get https://storage.googleapis.com/minikube/releases.json: x509: certificate signed by unknown authority

If I connect my mac to an outside network (such as Starbucks WIFI) the program runs successfully.

@quentinmit quentinmit added this to the Go1.8 milestone Jul 29, 2016
@josharian josharian changed the title crypto/x509 darwin only loads system.root keychain should also load system keychain crypto/x509: darwin only loads system.root keychain should also load system keychain Jul 31, 2016
@josharian
Copy link
Contributor

@josharian josharian commented Jul 31, 2016

The code quoted is for toolchains without cgo. Do you have cgo?

It could well be that the fix you suggest is correct, which would be great. I didn't find that file when I was working on https://codereview.appspot.com/22020045, and I experienced the exact same problem at the time, with a corp cert.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Jan 6, 2017

@jostockley, could you answer Josh's question? Are you using cgo?

@jostockley
Copy link
Author

@jostockley jostockley commented Jan 7, 2017

This was a while ago now, so I tried my test again. I'm using go installed on my Mac using homebrew.
How do I tell if its cgo or regular go??

$ more https.go
package main

import (
    "log"
    "net/http"
)

func main() {
    _, err := http.Get("https://storage.googleapis.com/minikube/releases.json")
    if err != nil {
        log.Fatal(err)
    }
}

$ go version
go version go1.7.4 darwin/amd64
$ go run https.go
2017/01/06 17:10:17 Get https://storage.googleapis.com/minikube/releases.json: x509: certificate signed by unknown authority
exit status 1
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Jan 7, 2017

When you filed a bug, the issue template requested that you paste the output of go env. That would contain the answer.

@jostockley
Copy link
Author

@jostockley jostockley commented Jan 7, 2017

So that is what the (go env) meant. It might be better if it said explicity to run the go env command.
Anyhow, here's the output from the go I have from homebrew:

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/stockj3/work"
GORACE=""
GOROOT="/usr/local/Cellar/go/1.7.4_1/libexec"
GOTOOLDIR="/usr/local/Cellar/go/1.7.4_1/libexec/pkg/tool/darwin_amd64"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/n2/9tk23c5d1bbb8tkpwz_w9pfmm97_88/T/go-build030846639=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Jan 7, 2017

Okay, so you have cgo.

I guess the next question is whether the changes during Go 1.8 helped this or not.

Could you try Go 1.8beta2? https://golang.org/dl/#go1.8beta2

$ go get golang.org/x/build/version/go1.8beta2
$ go1.8beta2 download
$ go1.8beta2 run https.go
@jostockley
Copy link
Author

@jostockley jostockley commented Jan 7, 2017

Problem still exists:

$ go1.8beta2 env
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/stockj3/work"
GORACE=""
GOROOT="/Users/stockj3/sdk/go1.8beta2"
GOTOOLDIR="/Users/stockj3/sdk/go1.8beta2/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/n2/9tk23c5d1bbb8tkpwz_w9pfmm97_88/T/go-build809517524=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"
PKG_CONFIG="pkg-config"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
$ go1.8beta2 run https.go
2017/01/06 17:42:26 Get https://storage.googleapis.com/minikube/releases.json: x509: certificate signed by unknown authority
exit status 1
@josharian
Copy link
Contributor

@josharian josharian commented Jan 7, 2017

I don't know much about this stuff, but if you wanted to experiment, the relevant code is in crypto/x509/root_cgo_darwin.go. One place to go sniffing might be the list of security domains we respect.

@mastercactapus
Copy link
Contributor

@mastercactapus mastercactapus commented Feb 11, 2017

Correct me if I'm wrong, but if you're not building against C, cgo won't be used (even if CGO_ENABLED=1) -- on darwin at least. A local test with CGO_ENABLED=0 and CGO_ENABLED=1 yields an identical binary.

The non-cgo cert code only looks at SystemRoots on darwin. It's missing the System and login keychains.

I dealt with this issue a lot at a previous employer (they did the same MITM-thing). For awhile we got away with adding the certs to the System Roots keychain, but newer versions of macOS prevent this. After that we just started using our own tls.Config.

Having said that, I honestly think this may be as simple as appending the missing paths to the /usr/bin/security command.
https://github.com/golang/go/blob/master/src/crypto/x509/root_darwin.go#L64

I'd be happy to make a CL if this seems sane to everyone.

@anoopbhat
Copy link

@anoopbhat anoopbhat commented Feb 11, 2017

I too just ran into this issue in mac os el capitan. The only place I can add a self signed cert is in

~/Library/Keychains/login.keychain

and

/Library/Keychains/System.keychain

Adding a cert using

sudo security add-trusted-cert -d -r trustRoot -k "/System/Library/Keychains/SystemRootCertificates.keychain" ~/cert.cer

results in SecCertificateAddToKeychain: UNIX[Operation not permitted]

So i think the above two keychains also need to be included. Or at the very least, the users login keychain but i don't know how difficult that would be. I'm not a go expert or a c expert so i'm just guessing here.

@mastercactapus
Copy link
Contributor

@mastercactapus mastercactapus commented Feb 11, 2017

I did some more testing this morning. The non-cgo solution is pretty simple and I can confirm it works, I can add a cert to the login.keychain on Sierra, trust it, and have it validate on an https request. Un-trust it and it doesn't, etc.. The same for the System keychain.

With cgo enabled, however, I'm getting ...signed by unknown authority (possibly because of "crypto/rsa: verification error" while trying to verify candidate authority certificate... -- so maybe there is a couple issues going on here.

mastercactapus added a commit to mastercactapus/go that referenced this issue Feb 11, 2017
The current implementation of loadSystemRoots does not
load all required (trusted) certificates in both the cgo
and nocgo paths.

In the nocgo path, certificates are simply not loaded from
the login or System keychains.

In the cgo path, certificates whos Subject doesn't match the
Issuer, are ignored. This is problematic in the case of a
enterprise environment with their own intermediate CAs. In
this case: the issuer is a separate root, which may be loaded,
but the intermediate is ignored. A TLS handshake may not
include the intermediate cert, leading to an error.

This change adds the System and login keychain files to the
nocgo path, and removes the restriction on Issuer and Subject
name matching in the cgo path.

Fixes golang#16532

Change-Id: I4786d6696b338c7e0e0c7806e5d0383f99d2db89
@anoopbhat
Copy link

@anoopbhat anoopbhat commented Feb 12, 2017

Hmm. I can't seem to recreate this.

I added the cert to my login keychain and System keychain and trusted both. Curl works as expected and so does Safari.

then i built my go binary with

export CGO_ENABLED=0; go build -v -o $GOPATH/bin/session ./cmd/session

This is my go env

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/anoopbhat/GO/gogo-cds-cli/"
GORACE=""
GOROOT="/usr/local/go"
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/04/3g8k53491bj5qr4f_r_q0x8w0000gp/T/go-build322740831=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="0"

The build is successful but here's the result

./session -a 172.24.50.44 -u username
Password: 
Get https://172.24.50.44/api/...: x509: cannot validate certificate for 172.24.50.44 because it doesn't contain any IP SANs

Maybe one of the packages i'm importing is using cgo and that could cause it?

i'll try just a plain old get of a page on that site.

edit: ok. figured out that certs with IP addresses are only validated using IP SAN certs. In my case, I'm screwed with getting this to work because I can't change the certificate but at least it's a problem on my end :)

@mastercactapus
Copy link
Contributor

@mastercactapus mastercactapus commented Feb 12, 2017

Just an update, I'm pretty confident that I have a fix ready for both cgo and nocgo. I'm attempting to validate that it fixes the issue in these environments. I'll make a CL once I can validate it.

@bradfitz in the cgo code path, it looks to be that the Issuer and Subject-match check doesn't account for environments with intermediate CAs that need to be on the chain. That's the general idea anyway; the CL commit will have a more detailed explanation/example once I "mail" it.

As for the nocgo: that code simply doesn't load the System and login keychains currently.

@jostockley would you be able to help validate the fix?

If anyone behind a MITM-type proxy would like to try validating, I made a gist to try to make validation simple as possible:
https://gist.github.com/mastercactapus/14554bc8379016e766bb0229cf386bf8

If you follow the script, it gets go 1.8, clones the fix, builds it, and runs an http.Get in go, printing the results of each.

When it's done, if all goes well, you should see:

CGO   1.8: 0
NOCGO 1.8: 0
CGO   fix: 1
NOCGO fix: 1

Meaning with 1.8 it should get a cert issue with both cgo/nocgo and with the fix, a successful https request for both.

@josharian
Copy link
Contributor

@josharian josharian commented Feb 13, 2017

@mastercactapus thanks for working on this! When you mail the CLs, please mail separate CLs for the cgo and non-cgo paths. A good reviewer would be agl@golang.org, and please CC me.

@jostockley
Copy link
Author

@jostockley jostockley commented Feb 13, 2017

@mastercactapus I ran your test script and here's the result:

CGO   1.8: 1
NOCGO 1.8: 1
CGO   fix: 1
NOCGO fix: 1

Not what I expected. I also tried my original test case and with your fix my test passes, without you fix my test fails. So that indicates that you fix works at least for my test case.

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 14, 2017

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

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 14, 2017

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

mastercactapus added a commit to mastercactapus/go that referenced this issue Feb 14, 2017
The current implementation ignores certificates that exist
in the login and System keychains.

This change adds the missing System and login keychain
files to the `/usr/bin/security` command in
`execSecurityRoots`. If the current user cannot be
obtained, the login keychain is ignored.

Refs golang#16532

Change-Id: I8594a6b8940c58df8a8015b274fa45c39e18862c
@goraxe
Copy link

@goraxe goraxe commented May 26, 2017

Hi, any news on this issue?

@josharian
Copy link
Contributor

@josharian josharian commented May 27, 2017

I think @quentinmit has reviewed some related code in the past. Quentin?

mastercactapus added a commit to mastercactapus/go that referenced this issue Jun 24, 2017
The current implementation ignores certs wherein the
Subject does not match the Issuer. An example of where
this causes issue is an enterprise environment with
intermediate CAs. In this case, the issuer is separate
(and may be loaded) but the intermediate is ignored.
A TLS handshake that does not include the intermediate
cert would then fail with an untrusted error in Go.

On other platforms (darwin-nocgo included), all trusted
certs are loaded and accepted reguardless of
Subject/Issuer names.

This change removes the Subject/Issuer name-matching
restriction of certificates, allowing all trusted certs
to be loaded on darwin (cgo).

Refs golang#16532

Change-Id: I451e929588f8911892be6bdc2143d0799363c5f8
mastercactapus added a commit to mastercactapus/go that referenced this issue Jun 25, 2017
The current implementation ignores certs wherein the
Subject does not match the Issuer. An example of where
this causes issue is an enterprise environment with
intermediate CAs. In this case, the issuer is separate
(and may be loaded) but the intermediate is ignored.
A TLS handshake that does not include the intermediate
cert would then fail with an untrusted error in Go.

On other platforms (darwin-nocgo included), all trusted
certs are loaded and accepted reguardless of
Subject/Issuer names.

This change removes the Subject/Issuer name-matching
restriction of certificates when trustAsRoot is set,
allowing all trusted certs to be loaded on darwin (cgo).

Refs golang#16532

Change-Id: I451e929588f8911892be6bdc2143d0799363c5f8
mastercactapus added a commit to mastercactapus/go that referenced this issue Jun 25, 2017
The current implementation ignores certificates that exist
in the login and System keychains.

This change adds the missing System and login keychain
files to the `/usr/bin/security` command in
`execSecurityRoots`. If the current user cannot be
obtained, the login keychain is ignored.

Refs golang#16532

Change-Id: I8594a6b8940c58df8a8015b274fa45c39e18862c
gopherbot pushed a commit that referenced this issue Jul 14, 2017
The current implementation ignores certificates that exist
in the login and System keychains.

This change adds the missing System and login keychain
files to the `/usr/bin/security` command in
`execSecurityRoots`. If the current user cannot be
obtained, the login keychain is ignored.

Refs #16532

Change-Id: I8594a6b8940c58df8a8015b274fa45c39e18862c
Reviewed-on: https://go-review.googlesource.com/36941
Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@bradfitz bradfitz modified the milestones: Go1.10, Go1.9 Jul 22, 2017
mastercactapus added a commit to mastercactapus/go that referenced this issue Aug 19, 2017
The current implementation ignores certs wherein the
Subject does not match the Issuer. An example of where
this causes issue is an enterprise environment with
intermediate CAs. In this case, the issuer is separate
(and may be loaded) but the intermediate is ignored.
A TLS handshake that does not include the intermediate
cert would then fail with an untrusted error in Go.

On other platforms (darwin-nocgo included), all trusted
certs are loaded and accepted reguardless of
Subject/Issuer names.

This change removes the Subject/Issuer name-matching
restriction of certificates when trustAsRoot is set,
allowing all trusted certs to be loaded on darwin (cgo).

Refs golang#16532

Change-Id: I451e929588f8911892be6bdc2143d0799363c5f8
@gopherbot
Copy link

@gopherbot gopherbot commented Aug 22, 2017

Change https://golang.org/cl/57830 mentions this issue: crypto/x509: skip TestSystemRoots

gopherbot pushed a commit that referenced this issue Aug 22, 2017
golang.org/cl/36941 enabled loading of all trusted certs on darwin
for the non-cgo execSecurityRoots.

The corresponding cgo version golang.org/cl/36942 for systemRootsPool
has not been merged yet.

This tests fails reliably on some darwin systems:
--- FAIL: TestSystemRoots (1.28s)
        root_darwin_test.go:31:     cgo sys roots: 353.552363ms
        root_darwin_test.go:32: non-cgo sys roots: 921.85297ms
        root_darwin_test.go:44: got 169 roots
        root_darwin_test.go:44: got 455 roots
        root_darwin_test.go:73: insufficient overlap between cgo and non-cgo roots; want at least 227, have 168
FAIL
FAIL    crypto/x509     2.445s

Updates #16532
Updates #21416

Change-Id: I52c2c847651fb3621fdb6ab858ebe8e28894c201
Reviewed-on: https://go-review.googlesource.com/57830
Run-TryBot: Martin Möhrmann <moehrmann@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Joe Tsai <thebrokentoaster@gmail.com>
@gopherbot
Copy link

@gopherbot gopherbot commented Sep 19, 2017

Change https://golang.org/cl/64851 mentions this issue: crypto/x509: add cgo for system keychain loading

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 15, 2017

Change https://golang.org/cl/70847 mentions this issue: [release-branch.go1.9] crypto/x509: skip TestSystemRoots

gopherbot pushed a commit that referenced this issue Oct 25, 2017
golang.org/cl/36941 enabled loading of all trusted certs on darwin
for the non-cgo execSecurityRoots.

The corresponding cgo version golang.org/cl/36942 for systemRootsPool
has not been merged yet.

This tests fails reliably on some darwin systems:
--- FAIL: TestSystemRoots (1.28s)
        root_darwin_test.go:31:     cgo sys roots: 353.552363ms
        root_darwin_test.go:32: non-cgo sys roots: 921.85297ms
        root_darwin_test.go:44: got 169 roots
        root_darwin_test.go:44: got 455 roots
        root_darwin_test.go:73: insufficient overlap between cgo and non-cgo roots; want at least 227, have 168
FAIL
FAIL    crypto/x509     2.445s

Updates #16532
Updates #21416

Change-Id: I52c2c847651fb3621fdb6ab858ebe8e28894c201
Reviewed-on: https://go-review.googlesource.com/57830
Run-TryBot: Martin Möhrmann <moehrmann@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Joe Tsai <thebrokentoaster@gmail.com>
Reviewed-on: https://go-review.googlesource.com/70847
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Martin Möhrmann <moehrmann@google.com>
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Dec 13, 2017

I believe this is fixed by https://go-review.googlesource.com/c/go/+/36942 which lacked the magic "Fixes" word.

@bradfitz bradfitz closed this Dec 13, 2017
gopherbot pushed a commit that referenced this issue Dec 13, 2017
The current implementation ignores certs wherein the
Subject does not match the Issuer. An example of where
this causes issue is an enterprise environment with
intermediate CAs. In this case, the issuer is separate
(and may be loaded) but the intermediate is ignored.
A TLS handshake that does not include the intermediate
cert would then fail with an untrusted error in Go.

On other platforms (darwin-nocgo included), all trusted
certs are loaded and accepted reguardless of
Subject/Issuer names.

This change removes the Subject/Issuer name-matching
restriction of certificates when trustAsRoot is set,
allowing all trusted certs to be loaded on darwin (cgo).

Refs #16532

Change-Id: I451e929588f8911892be6bdc2143d0799363c5f8
Reviewed-on: https://go-review.googlesource.com/36942
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@golang golang locked and limited conversation to collaborators Dec 13, 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
You can’t perform that action at this time.