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: root_cgo_darwin omits intermediate CAs with an empty policy settings or an unspecified trust type setting #30672

Closed
penglei opened this issue Mar 8, 2019 · 6 comments
Assignees
Milestone

Comments

@penglei
Copy link

@penglei penglei commented Mar 8, 2019

What version of Go are you using (go version)?

$ go version
go version go1.12 darwin/amd64

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/penglei/Library/Caches/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/penglei/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/Cellar/go/1.12/libexec"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.12/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/n1/b06cdpcn6y58vz_184h4rlbc0000gn/T/go-build500360568=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

  1. generate certs

    Use this script to generate a server certificate signed by an intermediate certificate

    https://gist.github.com/penglei/91530ced7174d4d96ecbe8a5f8420749

    this script will generate root.pem, root-key.pem, intermediate.pem, intermediate-key.pem, server.pem, server-key.pem.
    certs in following step can be found here.

  2. add ca cert to system by /usr/bin/security

    sudo security add-trusted-cert -d -r trustRoot -k /Library/Keychains/System.keychain   root.pem
    sudo security add-trusted-cert -d -r trustAsRoot -k /Library/Keychains/System.keychain  intermediate.pem
    

    NOTE: bug is produced when adding intermediate.pem by calling security add-trusted-cert with no any -p policy options.

  3. config a HTTPS server

    Copy server.pem and server-key.pem to nginx config directory

    add the following content to nginx.conf:

    server {
    	listen       443 ssl;
    	server_name  localhost;
    
    	ssl_certificate server.pem;
    	ssl_certificate_key server-key.pem;
    
    	location / {
    		root   html;
    		index  index.html index.htm;
    	}
    }
  4. run the following go program

    func main() {
    	_, err := http.DefaultClient.Get("https://127.0.0.1/")
    	if err != nil {
    		panic(err)
    	} else {
    		fmt.Println("OK")
    	}
    }

    please ensure CGO_ENABLED=1 (this is default)

What did you expect to see?

OK

What did you see instead?

panic: Get https://127.0.0.1/: x509: certificate signed by unknown authority
@gopherbot
Copy link

@gopherbot gopherbot commented Mar 8, 2019

Change https://golang.org/cl/166219 mentions this issue: fix root_cgo_darwin omits some trusty intermediate ca certificate

@FiloSottile FiloSottile changed the title root_cgo_darwin omits intermediate ca certificate with an empty policy settings or an *unspecified* trust type settings crypto/x509: root_cgo_darwin omits intermediate CAs with an empty policy settings or an unspecified trust type setting Mar 8, 2019
@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Mar 8, 2019

Probably a duplicate of #30471.

@FiloSottile FiloSottile added the NeedsFix label Mar 8, 2019
@FiloSottile FiloSottile added this to the Go1.13 milestone Mar 8, 2019
@FiloSottile FiloSottile self-assigned this Mar 8, 2019
@penglei
Copy link
Author

@penglei penglei commented Mar 11, 2019

Probably a duplicate of #30471.

Yes, it is. I reproduced the problem of #30471. After applying the patch and recompiling go binary, it's fixed.

@gopherbot
Copy link

@gopherbot gopherbot commented May 22, 2019

Change https://golang.org/cl/178539 mentions this issue: crypto/x509: include roots with empty or multiple policies on macOS

@gopherbot gopherbot closed this in 42bb476 May 22, 2019
@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented May 22, 2019

This should be now fixed at tip. Please test it with https://golang.org/dl/gotip and report back.

$ go get golang.org/dl/gotip
$ gotip download
$ GODEBUG=x509roots=1 gotip test -v -run TestSystemRoots crypto/x509
$ gotip run [YOUR_PROGRAM]
@gopherbot
Copy link

@gopherbot gopherbot commented Apr 14, 2020

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

gopherbot pushed a commit that referenced this issue May 7, 2020
+----------------------------------------------------------------------+
| Hello, if you are reading this and run macOS, please test this code: |
|                                                                      |
| $ GO111MODULE=on go get golang.org/dl/gotip@latest                   |
| $ gotip download                                              |
| $ GODEBUG=x509roots=1 gotip test crypto/x509 -v -run TestSystemRoots |
+----------------------------------------------------------------------+

We currently have two code paths to extract system roots on macOS: one
uses cgo to invoke a maze of Security.framework APIs; the other is a
horrible fallback that runs "/usr/bin/security verify-cert" on every
root that has custom policies to check if it's trusted for SSL.

The fallback is not only terrifying because it shells out to a binary,
but also because it lets in certificates that are not trusted roots but
are signed by trusted roots, and because it applies some filters (EKUs
and expiration) only to roots with custom policies, as the others are
not passed to verify-cert. The other code path, of course, requires cgo,
so can't be used when cross-compiling and involves a large ball of C.

It's all a mess, and it broke oh-so-many times (#14514, #16532, #19436,
 #20990, #21416, #24437, #24652, #25649, #26073, #27958, #28025, #28092,
 #29497, #30471, #30672, #30763, #30889, #32891, #38215, #38365, ...).

Since macOS does not have a stable syscall ABI, we already dynamically
link and invoke libSystem.dylib regardless of cgo availability (#17490).

How that works is that functions in package syscall (like syscall.Open)
take the address of assembly trampolines (like libc_open_trampoline)
that jump to symbols imported with cgo_import_dynamic (like libc_open),
and pass them along with arguments to syscall.syscall (which is
implemented as runtime.syscall_syscall). syscall_syscall informs the
scheduler and profiler, and then uses asmcgocall to switch to a system
stack and invoke runtime.syscall. The latter is an assembly trampoline
that unpacks the Go ABI arguments passed to syscall.syscall, finally
calls the remote function, and puts the return value on the Go stack.
(This last bit is the part that cgo compiles from a C wrapper.)

We can do something similar to link and invoke Security.framework!

The one difference is that runtime.syscall and friends check errors
based on the errno convention, which Security doesn't follow, so I added
runtime.syscallNoErr which just skips interpreting the return value.
We only need a variant with six arguments because the calling convention
is register-based, and extra arguments simply zero out some registers.

That's plumbed through as crypto/x509/internal/macOS.syscall. The rest
of that package is a set of wrappers for Security.framework and Core
Foundation functions, like syscall is for libSystem. In theory, as long
as macOS respects ABI backwards compatibility (a.k.a. as long as
binaries built for a previous OS version keep running) this should be
stable, as the final result is not different from what a C compiler
would make. (One exception might be dictionary key strings, which we
make our own copy of instead of using the dynamic symbol. If they change
the value of those strings things might break. But why would they.)

Finally, I rewrote the crypto/x509 cgo logic in Go using those wrappers.
It works! I tried to make it match 1:1 the old logic, so that
root_darwin_amd64.go can be reviewed by comparing it to
root_cgo_darwin_amd64.go. The only difference is that we do proper error
handling now, and assume that if there is no error the return values are
there, while before we'd just check for nil pointers and move on.

I kept the cgo logic to help with review and testing, but we should
delete it once we are confident the new code works.

The nocgo logic is gone and we shall never speak of it again.

Fixes #32604
Fixes #19561
Fixes #38365
Awakens Cthulhu

Change-Id: Id850962bad667f71e3af594bdfebbbb1edfbcbb4
Reviewed-on: https://go-review.googlesource.com/c/go/+/227037
Reviewed-by: Katie Hockman <katie@golang.org>
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.

3 participants
You can’t perform that action at this time.