From a2d01473aec0e9d19ff31bb9727dcc350d500e56 Mon Sep 17 00:00:00 2001 From: Michael Matloob Date: Tue, 3 Nov 2020 13:12:19 -0500 Subject: [PATCH 1/3] cmd/go: support cgo files in overlays This is a roll-forward of golang.org/cl/267197, which was reverted in golang.org/cl/267357. It makes the following changes in addition to the ones in the next paragraph: It avoids outputting trimpath arguments for an overlay unless the overlay affects the package being compiled (to avoid hitting windows command line argument limits), and it fixes processing of regexps in the script test framework to treat the first *non flag* argument to grep, stdout, and stderr as a regexp, not just the first argument. golang.org/cl/267917 was a roll-forward of golang.org/cl/262618, which was reverted in golang.org/cl/267037. The only differences between this CL and the original were the three calls to fflush from the C files in build_overlay.txt, to guarantee that the string we were expecting was actually written out. The CL requires rewriting the paths of the files passed to the cgo tool toolchain to use the overlaid paths instead of the disk paths of files. Because the directories of the overlaid paths don't exist in general, the cgo tool have been updated to run in base.Cwd instead of the package directory. For #39958 Change-Id: I1bd96db257564bcfd95b3502aeca14d04bd28618 Reviewed-on: https://go-review.googlesource.com/c/go/+/267797 Trust: Michael Matloob Trust: Bryan C. Mills Run-TryBot: Michael Matloob TryBot-Result: Go Bot Reviewed-by: Bryan C. Mills --- src/cmd/go/internal/work/exec.go | 48 +++++++++-- src/cmd/go/internal/work/gc.go | 45 ++++++++-- src/cmd/go/testdata/script/build_overlay.txt | 83 ++++++++++++++++++- .../go/testdata/script/build_trimpath_cgo.txt | 28 +++++++ 4 files changed, 189 insertions(+), 15 deletions(-) diff --git a/src/cmd/go/internal/work/exec.go b/src/cmd/go/internal/work/exec.go index 838b00a00d..a1a357e2ac 100644 --- a/src/cmd/go/internal/work/exec.go +++ b/src/cmd/go/internal/work/exec.go @@ -8,6 +8,7 @@ package work import ( "bytes" + "cmd/go/internal/fsys" "context" "encoding/json" "errors" @@ -2242,8 +2243,6 @@ func (b *Builder) ccompile(a *Action, p *load.Package, outfile string, flags []s // when -trimpath is enabled. if b.gccSupportsFlag(compiler, "-fdebug-prefix-map=a=b") { if cfg.BuildTrimpath { - // TODO(#39958): handle overlays - // Keep in sync with Action.trimpath. // The trimmed paths are a little different, but we need to trim in the // same situations. @@ -2313,7 +2312,8 @@ func (b *Builder) gccld(a *Action, p *load.Package, objdir, outfile string, flag cmdargs := []interface{}{cmd, "-o", outfile, objs, flags} dir := p.Dir - out, err := b.runOut(a, dir, b.cCompilerEnv(), cmdargs...) + out, err := b.runOut(a, base.Cwd, b.cCompilerEnv(), cmdargs...) + if len(out) > 0 { // Filter out useless linker warnings caused by bugs outside Go. // See also cmd/link/internal/ld's hostlink method. @@ -2641,7 +2641,8 @@ func (b *Builder) cgo(a *Action, cgoExe, objdir string, pcCFLAGS, pcLDFLAGS, cgo cgoLDFLAGS = append([]string{"-fsanitize=memory"}, cgoLDFLAGS...) } - // Allows including _cgo_export.h from .[ch] files in the package. + // Allows including _cgo_export.h, as well as the user's .h files, + // from .[ch] files in the package. cgoCPPFLAGS = append(cgoCPPFLAGS, "-I", objdir) // cgo @@ -2654,6 +2655,8 @@ func (b *Builder) cgo(a *Action, cgoExe, objdir string, pcCFLAGS, pcLDFLAGS, cgo cfiles = append(cfiles, f+".cgo2.c") } + hfiles := append([]string{}, p.HFiles...) + // TODO: make cgo not depend on $GOARCH? cgoflags := []string{} @@ -2698,7 +2701,38 @@ func (b *Builder) cgo(a *Action, cgoExe, objdir string, pcCFLAGS, pcLDFLAGS, cgo cgoflags = append(cgoflags, "-exportheader="+objdir+"_cgo_install.h") } - if err := b.run(a, p.Dir, p.ImportPath, cgoenv, cfg.BuildToolexec, cgoExe, "-objdir", objdir, "-importpath", p.ImportPath, cgoflags, "--", cgoCPPFLAGS, cgoCFLAGS, cgofiles); err != nil { + execdir := p.Dir + + // If any of the Cgo, C, or H files are overlaid, copy them all to + // objdir to ensure that they refer to the right header files. + // TODO(#39958): Ideally, we'd always do this, but this could + // subtly break some cgo files that include .h files across directory + // boundaries, even though they shouldn't. + hasOverlay := false + cgoFileLists := [][]string{cgofiles, gccfiles, gxxfiles, mfiles, ffiles, hfiles} +OverlayLoop: + for _, fs := range cgoFileLists { + for _, f := range fs { + if _, ok := fsys.OverlayPath(mkAbs(p.Dir, f)); ok { + hasOverlay = true + break OverlayLoop + } + } + } + if hasOverlay { + execdir = objdir + for _, fs := range cgoFileLists { + for i := range fs { + opath, _ := fsys.OverlayPath(mkAbs(p.Dir, fs[i])) + fs[i] = objdir + filepath.Base(fs[i]) + if err := b.copyFile(fs[i], opath, 0666, false); err != nil { + return nil, nil, err + } + } + } + } + + if err := b.run(a, execdir, p.ImportPath, cgoenv, cfg.BuildToolexec, cgoExe, "-objdir", objdir, "-importpath", p.ImportPath, cgoflags, "--", cgoCPPFLAGS, cgoCFLAGS, cgofiles); err != nil { return nil, nil, err } outGo = append(outGo, gofiles...) @@ -2792,7 +2826,7 @@ func (b *Builder) dynimport(a *Action, p *load.Package, objdir, importGo, cgoExe return err } - linkobj := str.StringList(ofile, outObj, p.SysoFiles) + linkobj := str.StringList(ofile, outObj, mkAbsFiles(p.Dir, p.SysoFiles)) dynobj := objdir + "_cgo_.o" // we need to use -pie for Linux/ARM to get accurate imported sym @@ -2817,7 +2851,7 @@ func (b *Builder) dynimport(a *Action, p *load.Package, objdir, importGo, cgoExe if p.Standard && p.ImportPath == "runtime/cgo" { cgoflags = []string{"-dynlinker"} // record path to dynamic linker } - return b.run(a, p.Dir, p.ImportPath, b.cCompilerEnv(), cfg.BuildToolexec, cgoExe, "-dynpackage", p.Name, "-dynimport", dynobj, "-dynout", importGo, cgoflags) + return b.run(a, base.Cwd, p.ImportPath, b.cCompilerEnv(), cfg.BuildToolexec, cgoExe, "-dynpackage", p.Name, "-dynimport", dynobj, "-dynout", importGo, cgoflags) } // Run SWIG on all SWIG input files. diff --git a/src/cmd/go/internal/work/gc.go b/src/cmd/go/internal/work/gc.go index e79173485d..56711b52d8 100644 --- a/src/cmd/go/internal/work/gc.go +++ b/src/cmd/go/internal/work/gc.go @@ -262,7 +262,7 @@ func (a *Action) trimpath() string { if len(objdir) > 1 && objdir[len(objdir)-1] == filepath.Separator { objdir = objdir[:len(objdir)-1] } - rewrite := objdir + "=>" + rewrite := "" rewriteDir := a.Package.Dir if cfg.BuildTrimpath { @@ -271,21 +271,54 @@ func (a *Action) trimpath() string { } else { rewriteDir = a.Package.ImportPath } - rewrite += ";" + a.Package.Dir + "=>" + rewriteDir + rewrite += a.Package.Dir + "=>" + rewriteDir + ";" } // Add rewrites for overlays. The 'from' and 'to' paths in overlays don't need to have // same basename, so go from the overlay contents file path (passed to the compiler) // to the path the disk path would be rewritten to. + + cgoFiles := make(map[string]bool) + for _, f := range a.Package.CgoFiles { + cgoFiles[f] = true + } + + // TODO(matloob): Higher up in the stack, when the logic for deciding when to make copies + // of c/c++/m/f/hfiles is consolidated, use the same logic that Build uses to determine + // whether to create the copies in objdir to decide whether to rewrite objdir to the + // package directory here. + var overlayNonGoRewrites string // rewrites for non-go files + hasCgoOverlay := false if fsys.OverlayFile != "" { for _, filename := range a.Package.AllFiles() { - overlayPath, ok := fsys.OverlayPath(filepath.Join(a.Package.Dir, filename)) - if !ok { - continue + path := filename + if !filepath.IsAbs(path) { + path = filepath.Join(a.Package.Dir, path) + } + base := filepath.Base(path) + isGo := strings.HasSuffix(filename, ".go") || strings.HasSuffix(filename, ".s") + isCgo := cgoFiles[filename] || !isGo + overlayPath, isOverlay := fsys.OverlayPath(path) + if isCgo && isOverlay { + hasCgoOverlay = true + } + if !isCgo && isOverlay { + rewrite += overlayPath + "=>" + filepath.Join(rewriteDir, base) + ";" + } else if isCgo { + // Generate rewrites for non-Go files copied to files in objdir. + if filepath.Dir(path) == a.Package.Dir { + // This is a file copied to objdir. + overlayNonGoRewrites += filepath.Join(objdir, base) + "=>" + filepath.Join(rewriteDir, base) + ";" + } + } else { + // Non-overlay Go files are covered by the a.Package.Dir rewrite rule above. } - rewrite += ";" + overlayPath + "=>" + filepath.Join(rewriteDir, filename) } } + if hasCgoOverlay { + rewrite += overlayNonGoRewrites + } + rewrite += objdir + "=>" return rewrite } diff --git a/src/cmd/go/testdata/script/build_overlay.txt b/src/cmd/go/testdata/script/build_overlay.txt index 0602e706e9..e18a8f5b28 100644 --- a/src/cmd/go/testdata/script/build_overlay.txt +++ b/src/cmd/go/testdata/script/build_overlay.txt @@ -1,9 +1,11 @@ [short] skip # Test building in overlays. -# TODO(matloob): add a test case where the destination file in the replace map +# TODO(#39958): add a test case where the destination file in the replace map # isn't a go file. Either completely exclude that case in fs.IsDirWithGoFiles # if the compiler doesn't allow it, or test that it works all the way. +# TODO(#39958): add a test that both gc and gccgo assembly files can include .h +# files. # The main package (m) is contained in an overlay. It imports m/dir2 which has one # file in an overlay and one file outside the overlay, which in turn imports m/dir, @@ -29,6 +31,18 @@ exec ./print_trimpath_two_files$GOEXE stdout $WORK[/\\]gopath[/\\]src[/\\]m[/\\]printpath[/\\]main.go stdout $WORK[/\\]gopath[/\\]src[/\\]m[/\\]printpath[/\\]other.go +go build -overlay overlay.json -o main_cgo_replace$GOEXE ./cgo_hello_replace +exec ./main_cgo_replace$GOEXE +stdout '^hello cgo\r?\n' + +go build -overlay overlay.json -o main_cgo_quote$GOEXE ./cgo_hello_quote +exec ./main_cgo_quote$GOEXE +stdout '^hello cgo\r?\n' + +go build -overlay overlay.json -o main_cgo_angle$GOEXE ./cgo_hello_angle +exec ./main_cgo_angle$GOEXE +stdout '^hello cgo\r?\n' + # Run same tests but with gccgo. env GO111MODULE=off [!exec:gccgo] stop @@ -46,6 +60,19 @@ go build -compiler=gccgo -overlay overlay.json -o print_trimpath_gccgo$GOEXE -tr exec ./print_trimpath_gccgo$GOEXE stdout ^\.[/\\]printpath[/\\]main.go + +go build -compiler=gccgo -overlay overlay.json -o main_cgo_replace_gccgo$GOEXE ./cgo_hello_replace +exec ./main_cgo_replace_gccgo$GOEXE +stdout '^hello cgo\r?\n' + +go build -compiler=gccgo -overlay overlay.json -o main_cgo_quote_gccgo$GOEXE ./cgo_hello_quote +exec ./main_cgo_quote_gccgo$GOEXE +stdout '^hello cgo\r?\n' + +go build -compiler=gccgo -overlay overlay.json -o main_cgo_angle_gccgo$GOEXE ./cgo_hello_angle +exec ./main_cgo_angle_gccgo$GOEXE +stdout '^hello cgo\r?\n' + -- m/go.mod -- // TODO(matloob): how do overlays work with go.mod (especially if mod=readonly) module m @@ -71,9 +98,32 @@ the actual code is in the overlay "dir/g.go": "overlay/dir_g.go", "dir2/i.go": "overlay/dir2_i.go", "printpath/main.go": "overlay/printpath.go", - "printpath/other.go": "overlay2/printpath2.go" + "printpath/other.go": "overlay2/printpath2.go", + "cgo_hello_replace/cgo_header.h": "overlay/cgo_head.h", + "cgo_hello_quote/cgo_hello.go": "overlay/cgo_hello_quote.go", + "cgo_hello_quote/cgo_header.h": "overlay/cgo_head.h", + "cgo_hello_angle/cgo_hello.go": "overlay/cgo_hello_angle.go", + "cgo_hello_angle/cgo_header.h": "overlay/cgo_head.h" } } +-- m/cgo_hello_replace/cgo_hello_replace.go -- +package main + +// #include "cgo_header.h" +import "C" + +func main() { + C.say_hello() +} +-- m/cgo_hello_replace/cgo_header.h -- + // Test that this header is replaced with one that has the proper declaration. +void say_goodbye(); + +-- m/cgo_hello_replace/goodbye.c -- +#include + +void say_hello() { puts("hello cgo\n"); fflush(stdout); } + -- m/overlay/f.go -- package main @@ -128,3 +178,32 @@ import "m/dir" func printMessage() { dir.PrintMessage() } +-- m/overlay/cgo_hello_quote.go -- +package main + +// #include "cgo_header.h" +import "C" + +func main() { + C.say_hello() +} +-- m/overlay/cgo_hello_angle.go -- +package main + +// #include +import "C" + +func main() { + C.say_hello() +} +-- m/overlay/cgo_head.h -- +void say_hello(); +-- m/cgo_hello_quote/hello.c -- +#include + +void say_hello() { puts("hello cgo\n"); fflush(stdout); } +-- m/cgo_hello_angle/hello.c -- +#include + +void say_hello() { puts("hello cgo\n"); fflush(stdout); } + diff --git a/src/cmd/go/testdata/script/build_trimpath_cgo.txt b/src/cmd/go/testdata/script/build_trimpath_cgo.txt index 4608d9ac6b..3187b4d643 100644 --- a/src/cmd/go/testdata/script/build_trimpath_cgo.txt +++ b/src/cmd/go/testdata/script/build_trimpath_cgo.txt @@ -20,10 +20,38 @@ go build -trimpath -o hello.exe . go run ./list-dwarf hello.exe ! stdout gopath/src + +# Do the above, with the cgo (but not .c) sources in an overlay +# Check that the source path appears when -trimpath is not used. +mkdir $WORK/overlay +cp hello.go $WORK/overlay/hello.go +mkdir hello_overlay +cp hello.c hello_overlay/hello.c +go build -overlay overlay.json -o hello_overlay.exe ./hello_overlay +grep -q gopath[/\\]src hello_overlay.exe +! grep -q $WORK[/\\]overlay hello_overlay.exe +go run ./list-dwarf hello_overlay.exe +stdout gopath[/\\]src +! stdout $WORK[/\\]overlay + +# Check that the source path does not appear when -trimpath is used. +go build -overlay overlay.json -trimpath -o hello_overlay.exe ./hello_overlay +! grep -q gopath[/\\]src hello_overlay.exe +! grep -q $WORK[/\\]overlay hello_overlay.exe +go run ./list-dwarf hello_overlay.exe +! stdout gopath/src +! stdout $WORK[/\\]overlay + -- go.mod -- module m go 1.14 +-- overlay.json -- +{ + "Replace": { + "hello_overlay/hello.go": "../../overlay/hello.go" + } +} -- hello.c -- #include From d7fff1f2cf2c0cb7cb2e03a3d057c600c4ec545a Mon Sep 17 00:00:00 2001 From: Filippo Valsorda Date: Wed, 24 Jun 2020 17:01:00 -0400 Subject: [PATCH 2/3] crypto/tls: ensure the server picked an advertised ALPN protocol This is a SHALL in RFC 7301, Section 3.2. Also some more cleanup after NPN, which worked the other way around (with the possibility that the client could pick a protocol the server did not suggest). Change-Id: I83cc43ca1b3c686dfece8315436441c077065d82 Reviewed-on: https://go-review.googlesource.com/c/go/+/239748 Run-TryBot: Filippo Valsorda TryBot-Result: Go Bot Trust: Filippo Valsorda Trust: Roland Shoemaker Reviewed-by: Roland Shoemaker --- doc/go1.16.html | 8 ++++++ src/crypto/tls/common.go | 3 --- src/crypto/tls/conn.go | 6 ++--- src/crypto/tls/handshake_client.go | 33 +++++++++++------------- src/crypto/tls/handshake_client_tls13.go | 14 +++++++--- src/crypto/tls/handshake_server.go | 2 +- src/crypto/tls/handshake_server_tls13.go | 2 +- 7 files changed, 38 insertions(+), 30 deletions(-) diff --git a/doc/go1.16.html b/doc/go1.16.html index bb920a0cb8..43ffb9dd7c 100644 --- a/doc/go1.16.html +++ b/doc/go1.16.html @@ -286,6 +286,14 @@ Do not send CLs removing the interior tags from such phrases. has no effect.

+

+ Clients now ensure that the server selects + + an ALPN protocol from + + the list advertised by the client. +

+

crypto/x509

diff --git a/src/crypto/tls/common.go b/src/crypto/tls/common.go index 1370d26fe2..98b31b09fa 100644 --- a/src/crypto/tls/common.go +++ b/src/crypto/tls/common.go @@ -229,9 +229,6 @@ type ConnectionState struct { CipherSuite uint16 // NegotiatedProtocol is the application protocol negotiated with ALPN. - // - // Note that on the client side, this is currently not guaranteed to be from - // Config.NextProtos. NegotiatedProtocol string // NegotiatedProtocolIsMutual used to indicate a mutual NPN negotiation. diff --git a/src/crypto/tls/conn.go b/src/crypto/tls/conn.go index 2788c3c393..969f357834 100644 --- a/src/crypto/tls/conn.go +++ b/src/crypto/tls/conn.go @@ -88,8 +88,8 @@ type Conn struct { clientFinished [12]byte serverFinished [12]byte - clientProtocol string - clientProtocolFallback bool + // clientProtocol is the negotiated ALPN protocol. + clientProtocol string // input/output in, out halfConn @@ -1471,7 +1471,7 @@ func (c *Conn) connectionStateLocked() ConnectionState { state.Version = c.vers state.NegotiatedProtocol = c.clientProtocol state.DidResume = c.didResume - state.NegotiatedProtocolIsMutual = !c.clientProtocolFallback + state.NegotiatedProtocolIsMutual = true state.ServerName = c.serverName state.CipherSuite = c.cipherSuite state.PeerCertificates = c.peerCertificates diff --git a/src/crypto/tls/handshake_client.go b/src/crypto/tls/handshake_client.go index 123df7b07a..92e33e7169 100644 --- a/src/crypto/tls/handshake_client.go +++ b/src/crypto/tls/handshake_client.go @@ -705,18 +705,18 @@ func (hs *clientHandshakeState) processServerHello() (bool, error) { } } - clientDidALPN := len(hs.hello.alpnProtocols) > 0 - serverHasALPN := len(hs.serverHello.alpnProtocol) > 0 - - if !clientDidALPN && serverHasALPN { - c.sendAlert(alertHandshakeFailure) - return false, errors.New("tls: server advertised unrequested ALPN extension") - } - - if serverHasALPN { + if hs.serverHello.alpnProtocol != "" { + if len(hs.hello.alpnProtocols) == 0 { + c.sendAlert(alertUnsupportedExtension) + return false, errors.New("tls: server advertised unrequested ALPN extension") + } + if mutualProtocol([]string{hs.serverHello.alpnProtocol}, hs.hello.alpnProtocols) == "" { + c.sendAlert(alertUnsupportedExtension) + return false, errors.New("tls: server selected unadvertised ALPN protocol") + } c.clientProtocol = hs.serverHello.alpnProtocol - c.clientProtocolFallback = false } + c.scts = hs.serverHello.scts if !hs.serverResumedSession() { @@ -973,20 +973,17 @@ func clientSessionCacheKey(serverAddr net.Addr, config *Config) string { return serverAddr.String() } -// mutualProtocol finds the mutual Next Protocol Negotiation or ALPN protocol -// given list of possible protocols and a list of the preference order. The -// first list must not be empty. It returns the resulting protocol and flag -// indicating if the fallback case was reached. -func mutualProtocol(protos, preferenceProtos []string) (string, bool) { +// mutualProtocol finds the mutual ALPN protocol given list of possible +// protocols and a list of the preference order. +func mutualProtocol(protos, preferenceProtos []string) string { for _, s := range preferenceProtos { for _, c := range protos { if s == c { - return s, false + return s } } } - - return protos[0], true + return "" } // hostnameInSNI converts name into an appropriate hostname for SNI. diff --git a/src/crypto/tls/handshake_client_tls13.go b/src/crypto/tls/handshake_client_tls13.go index 0e4b380035..be37c681c6 100644 --- a/src/crypto/tls/handshake_client_tls13.go +++ b/src/crypto/tls/handshake_client_tls13.go @@ -396,11 +396,17 @@ func (hs *clientHandshakeStateTLS13) readServerParameters() error { } hs.transcript.Write(encryptedExtensions.marshal()) - if len(encryptedExtensions.alpnProtocol) != 0 && len(hs.hello.alpnProtocols) == 0 { - c.sendAlert(alertUnsupportedExtension) - return errors.New("tls: server advertised unrequested ALPN extension") + if encryptedExtensions.alpnProtocol != "" { + if len(hs.hello.alpnProtocols) == 0 { + c.sendAlert(alertUnsupportedExtension) + return errors.New("tls: server advertised unrequested ALPN extension") + } + if mutualProtocol([]string{encryptedExtensions.alpnProtocol}, hs.hello.alpnProtocols) == "" { + c.sendAlert(alertUnsupportedExtension) + return errors.New("tls: server selected unadvertised ALPN protocol") + } + c.clientProtocol = encryptedExtensions.alpnProtocol } - c.clientProtocol = encryptedExtensions.alpnProtocol return nil } diff --git a/src/crypto/tls/handshake_server.go b/src/crypto/tls/handshake_server.go index 73df19d10f..a7d44144cb 100644 --- a/src/crypto/tls/handshake_server.go +++ b/src/crypto/tls/handshake_server.go @@ -218,7 +218,7 @@ func (hs *serverHandshakeState) processClientHello() error { } if len(hs.clientHello.alpnProtocols) > 0 { - if selectedProto, fallback := mutualProtocol(hs.clientHello.alpnProtocols, c.config.NextProtos); !fallback { + if selectedProto := mutualProtocol(hs.clientHello.alpnProtocols, c.config.NextProtos); selectedProto != "" { hs.hello.alpnProtocol = selectedProto c.clientProtocol = selectedProto } diff --git a/src/crypto/tls/handshake_server_tls13.go b/src/crypto/tls/handshake_server_tls13.go index 25c37b92c5..41f7ac2324 100644 --- a/src/crypto/tls/handshake_server_tls13.go +++ b/src/crypto/tls/handshake_server_tls13.go @@ -555,7 +555,7 @@ func (hs *serverHandshakeStateTLS13) sendServerParameters() error { encryptedExtensions := new(encryptedExtensionsMsg) if len(hs.clientHello.alpnProtocols) > 0 { - if selectedProto, fallback := mutualProtocol(hs.clientHello.alpnProtocols, c.config.NextProtos); !fallback { + if selectedProto := mutualProtocol(hs.clientHello.alpnProtocols, c.config.NextProtos); selectedProto != "" { encryptedExtensions.alpnProtocol = selectedProto c.clientProtocol = selectedProto } From 5e181357c7dd5cde11b28b2db0b4fb02159ddacf Mon Sep 17 00:00:00 2001 From: Filippo Valsorda Date: Mon, 9 Nov 2020 14:54:55 +0100 Subject: [PATCH 3/3] crypto/x509: drop the cgo implementation of root_darwin_amd64.go This code was preserved just to do side-by-side testing while transitioning to the Go implementation. There haven't been mismatch issues, so drop the cgo code, which was making it hard to improve the Go code without diverging. Change-Id: I2a23039c31a46e88b94250aafbc98d4ea8daf22f Reviewed-on: https://go-review.googlesource.com/c/go/+/232397 Run-TryBot: Filippo Valsorda TryBot-Result: Go Bot Trust: Filippo Valsorda Reviewed-by: Brad Fitzpatrick --- src/cmd/dist/test.go | 1 - .../x509/internal/macos/corefoundation.go | 4 + src/crypto/x509/internal/macos/security.go | 4 + src/crypto/x509/root_cgo_darwin.go | 326 ------------------ src/crypto/x509/root_darwin.go | 4 - src/crypto/x509/root_darwin_test.go | 33 -- src/crypto/x509/root_ios.go | 3 - src/crypto/x509/root_ios_gen.go | 3 - src/crypto/x509/root_omit.go | 3 - src/go/build/deps_test.go | 2 +- 10 files changed, 9 insertions(+), 374 deletions(-) delete mode 100644 src/crypto/x509/root_cgo_darwin.go diff --git a/src/cmd/dist/test.go b/src/cmd/dist/test.go index 5e3711b0c8..2a17ab9cae 100644 --- a/src/cmd/dist/test.go +++ b/src/cmd/dist/test.go @@ -1444,7 +1444,6 @@ func (t *tester) testDirTest(dt *distTest, shard, shards int) error { // cgoPackages is the standard packages that use cgo. var cgoPackages = []string{ - "crypto/x509", "net", "os/user", } diff --git a/src/crypto/x509/internal/macos/corefoundation.go b/src/crypto/x509/internal/macos/corefoundation.go index a248ee3292..9b776d4b85 100644 --- a/src/crypto/x509/internal/macos/corefoundation.go +++ b/src/crypto/x509/internal/macos/corefoundation.go @@ -16,6 +16,10 @@ import ( "unsafe" ) +// Core Foundation linker flags for the external linker. See Issue 42459. +//go:cgo_ldflag "-framework" +//go:cgo_ldflag "CoreFoundation" + // CFRef is an opaque reference to a Core Foundation object. It is a pointer, // but to memory not owned by Go, so not an unsafe.Pointer. type CFRef uintptr diff --git a/src/crypto/x509/internal/macos/security.go b/src/crypto/x509/internal/macos/security.go index 59cc19c587..5e39e93666 100644 --- a/src/crypto/x509/internal/macos/security.go +++ b/src/crypto/x509/internal/macos/security.go @@ -12,6 +12,10 @@ import ( "unsafe" ) +// Security.framework linker flags for the external linker. See Issue 42459. +//go:cgo_ldflag "-framework" +//go:cgo_ldflag "Security" + // Based on https://opensource.apple.com/source/Security/Security-59306.41.2/base/Security.h type SecTrustSettingsResult int32 diff --git a/src/crypto/x509/root_cgo_darwin.go b/src/crypto/x509/root_cgo_darwin.go deleted file mode 100644 index 825e8d4812..0000000000 --- a/src/crypto/x509/root_cgo_darwin.go +++ /dev/null @@ -1,326 +0,0 @@ -// Copyright 2011 The Go Authors. All rights reserved. -// Use of this source code is governed by a BSD-style -// license that can be found in the LICENSE file. - -// +build !ios - -package x509 - -// This cgo implementation exists only to support side-by-side testing by -// TestSystemRoots. It can be removed once we are confident in the no-cgo -// implementation. - -/* -#cgo CFLAGS: -mmacosx-version-min=10.11 -#cgo LDFLAGS: -framework CoreFoundation -framework Security - -#include -#include - -#include -#include - -static Boolean isSSLPolicy(SecPolicyRef policyRef) { - if (!policyRef) { - return false; - } - CFDictionaryRef properties = SecPolicyCopyProperties(policyRef); - if (properties == NULL) { - return false; - } - Boolean isSSL = false; - CFTypeRef value = NULL; - if (CFDictionaryGetValueIfPresent(properties, kSecPolicyOid, (const void **)&value)) { - isSSL = CFEqual(value, kSecPolicyAppleSSL); - } - CFRelease(properties); - return isSSL; -} - -// sslTrustSettingsResult obtains the final kSecTrustSettingsResult value -// for a certificate in the user or admin domain, combining usage constraints -// for the SSL SecTrustSettingsPolicy, ignoring SecTrustSettingsKeyUsage and -// kSecTrustSettingsAllowedError. -// https://developer.apple.com/documentation/security/1400261-sectrustsettingscopytrustsetting -static SInt32 sslTrustSettingsResult(SecCertificateRef cert) { - CFArrayRef trustSettings = NULL; - OSStatus err = SecTrustSettingsCopyTrustSettings(cert, kSecTrustSettingsDomainUser, &trustSettings); - - // According to Apple's SecTrustServer.c, "user trust settings overrule admin trust settings", - // but the rules of the override are unclear. Let's assume admin trust settings are applicable - // if and only if user trust settings fail to load or are NULL. - if (err != errSecSuccess || trustSettings == NULL) { - if (trustSettings != NULL) CFRelease(trustSettings); - err = SecTrustSettingsCopyTrustSettings(cert, kSecTrustSettingsDomainAdmin, &trustSettings); - } - - // > no trust settings [...] means "this certificate must be verified to a known trusted certificate” - // (Should this cause a fallback from user to admin domain? It's unclear.) - if (err != errSecSuccess || trustSettings == NULL) { - if (trustSettings != NULL) CFRelease(trustSettings); - return kSecTrustSettingsResultUnspecified; - } - - // > An empty trust settings array means "always trust this certificate” with an - // > overall trust setting for the certificate of kSecTrustSettingsResultTrustRoot. - if (CFArrayGetCount(trustSettings) == 0) { - CFRelease(trustSettings); - return kSecTrustSettingsResultTrustRoot; - } - - // kSecTrustSettingsResult is defined as CFSTR("kSecTrustSettingsResult"), - // but the Go linker's internal linking mode can't handle CFSTR relocations. - // Create our own dynamic string instead and release it below. - CFStringRef _kSecTrustSettingsResult = CFStringCreateWithCString( - NULL, "kSecTrustSettingsResult", kCFStringEncodingUTF8); - CFStringRef _kSecTrustSettingsPolicy = CFStringCreateWithCString( - NULL, "kSecTrustSettingsPolicy", kCFStringEncodingUTF8); - CFStringRef _kSecTrustSettingsPolicyString = CFStringCreateWithCString( - NULL, "kSecTrustSettingsPolicyString", kCFStringEncodingUTF8); - - CFIndex m; SInt32 result = 0; - for (m = 0; m < CFArrayGetCount(trustSettings); m++) { - CFDictionaryRef tSetting = (CFDictionaryRef)CFArrayGetValueAtIndex(trustSettings, m); - - // First, check if this trust setting is constrained to a non-SSL policy. - SecPolicyRef policyRef; - if (CFDictionaryGetValueIfPresent(tSetting, _kSecTrustSettingsPolicy, (const void**)&policyRef)) { - if (!isSSLPolicy(policyRef)) { - continue; - } - } - - if (CFDictionaryContainsKey(tSetting, _kSecTrustSettingsPolicyString)) { - // Restricted to a hostname, not a root. - continue; - } - - CFNumberRef cfNum; - if (CFDictionaryGetValueIfPresent(tSetting, _kSecTrustSettingsResult, (const void**)&cfNum)) { - CFNumberGetValue(cfNum, kCFNumberSInt32Type, &result); - } else { - // > If this key is not present, a default value of - // > kSecTrustSettingsResultTrustRoot is assumed. - result = kSecTrustSettingsResultTrustRoot; - } - - // If multiple dictionaries match, we are supposed to "OR" them, - // the semantics of which are not clear. Since TrustRoot and TrustAsRoot - // are mutually exclusive, Deny should probably override, and Invalid and - // Unspecified be overridden, approximate this by stopping at the first - // TrustRoot, TrustAsRoot or Deny. - if (result == kSecTrustSettingsResultTrustRoot) { - break; - } else if (result == kSecTrustSettingsResultTrustAsRoot) { - break; - } else if (result == kSecTrustSettingsResultDeny) { - break; - } - } - - // If trust settings are present, but none of them match the policy... - // the docs don't tell us what to do. - // - // "Trust settings for a given use apply if any of the dictionaries in the - // certificate’s trust settings array satisfies the specified use." suggests - // that it's as if there were no trust settings at all, so we should probably - // fallback to the admin trust settings. TODO. - if (result == 0) { - result = kSecTrustSettingsResultUnspecified; - } - - CFRelease(_kSecTrustSettingsPolicy); - CFRelease(_kSecTrustSettingsPolicyString); - CFRelease(_kSecTrustSettingsResult); - CFRelease(trustSettings); - - return result; -} - -// isRootCertificate reports whether Subject and Issuer match. -static Boolean isRootCertificate(SecCertificateRef cert, CFErrorRef *errRef) { - CFDataRef subjectName = SecCertificateCopyNormalizedSubjectContent(cert, errRef); - if (*errRef != NULL) { - return false; - } - CFDataRef issuerName = SecCertificateCopyNormalizedIssuerContent(cert, errRef); - if (*errRef != NULL) { - CFRelease(subjectName); - return false; - } - Boolean equal = CFEqual(subjectName, issuerName); - CFRelease(subjectName); - CFRelease(issuerName); - return equal; -} - -// CopyPEMRoots fetches the system's list of trusted X.509 root certificates -// for the kSecTrustSettingsPolicy SSL. -// -// On success it returns 0 and fills pemRoots with a CFDataRef that contains the extracted root -// certificates of the system. On failure, the function returns -1. -// Additionally, it fills untrustedPemRoots with certs that must be removed from pemRoots. -// -// Note: The CFDataRef returned in pemRoots and untrustedPemRoots must -// be released (using CFRelease) after we've consumed its content. -static int CopyPEMRoots(CFDataRef *pemRoots, CFDataRef *untrustedPemRoots, bool debugDarwinRoots) { - int i; - - if (debugDarwinRoots) { - fprintf(stderr, "crypto/x509: kSecTrustSettingsResultInvalid = %d\n", kSecTrustSettingsResultInvalid); - fprintf(stderr, "crypto/x509: kSecTrustSettingsResultTrustRoot = %d\n", kSecTrustSettingsResultTrustRoot); - fprintf(stderr, "crypto/x509: kSecTrustSettingsResultTrustAsRoot = %d\n", kSecTrustSettingsResultTrustAsRoot); - fprintf(stderr, "crypto/x509: kSecTrustSettingsResultDeny = %d\n", kSecTrustSettingsResultDeny); - fprintf(stderr, "crypto/x509: kSecTrustSettingsResultUnspecified = %d\n", kSecTrustSettingsResultUnspecified); - } - - // Get certificates from all domains, not just System, this lets - // the user add CAs to their "login" keychain, and Admins to add - // to the "System" keychain - SecTrustSettingsDomain domains[] = { kSecTrustSettingsDomainSystem, - kSecTrustSettingsDomainAdmin, kSecTrustSettingsDomainUser }; - - int numDomains = sizeof(domains)/sizeof(SecTrustSettingsDomain); - if (pemRoots == NULL || untrustedPemRoots == NULL) { - return -1; - } - - CFMutableDataRef combinedData = CFDataCreateMutable(kCFAllocatorDefault, 0); - CFMutableDataRef combinedUntrustedData = CFDataCreateMutable(kCFAllocatorDefault, 0); - for (i = 0; i < numDomains; i++) { - int j; - CFArrayRef certs = NULL; - OSStatus err = SecTrustSettingsCopyCertificates(domains[i], &certs); - if (err != noErr) { - continue; - } - - CFIndex numCerts = CFArrayGetCount(certs); - for (j = 0; j < numCerts; j++) { - SecCertificateRef cert = (SecCertificateRef)CFArrayGetValueAtIndex(certs, j); - if (cert == NULL) { - continue; - } - - SInt32 result; - if (domains[i] == kSecTrustSettingsDomainSystem) { - // Certs found in the system domain are always trusted. If the user - // configures "Never Trust" on such a cert, it will also be found in the - // admin or user domain, causing it to be added to untrustedPemRoots. The - // Go code will then clean this up. - result = kSecTrustSettingsResultTrustRoot; - } else { - result = sslTrustSettingsResult(cert); - if (debugDarwinRoots) { - CFErrorRef errRef = NULL; - CFStringRef summary = SecCertificateCopyShortDescription(NULL, cert, &errRef); - if (errRef != NULL) { - fprintf(stderr, "crypto/x509: SecCertificateCopyShortDescription failed\n"); - CFRelease(errRef); - continue; - } - - CFIndex length = CFStringGetLength(summary); - CFIndex maxSize = CFStringGetMaximumSizeForEncoding(length, kCFStringEncodingUTF8) + 1; - char *buffer = malloc(maxSize); - if (CFStringGetCString(summary, buffer, maxSize, kCFStringEncodingUTF8)) { - fprintf(stderr, "crypto/x509: %s returned %d\n", buffer, (int)result); - } - free(buffer); - CFRelease(summary); - } - } - - CFMutableDataRef appendTo; - // > Note the distinction between the results kSecTrustSettingsResultTrustRoot - // > and kSecTrustSettingsResultTrustAsRoot: The former can only be applied to - // > root (self-signed) certificates; the latter can only be applied to - // > non-root certificates. - if (result == kSecTrustSettingsResultTrustRoot) { - CFErrorRef errRef = NULL; - if (!isRootCertificate(cert, &errRef) || errRef != NULL) { - if (errRef != NULL) CFRelease(errRef); - continue; - } - - appendTo = combinedData; - } else if (result == kSecTrustSettingsResultTrustAsRoot) { - CFErrorRef errRef = NULL; - if (isRootCertificate(cert, &errRef) || errRef != NULL) { - if (errRef != NULL) CFRelease(errRef); - continue; - } - - appendTo = combinedData; - } else if (result == kSecTrustSettingsResultDeny) { - appendTo = combinedUntrustedData; - } else if (result == kSecTrustSettingsResultUnspecified) { - // Certificates with unspecified trust should probably be added to a pool of - // intermediates for chain building, or checked for transitive trust and - // added to the root pool (which is an imprecise approximation because it - // cuts chains short) but we don't support either at the moment. TODO. - continue; - } else { - continue; - } - - CFDataRef data = NULL; - err = SecItemExport(cert, kSecFormatX509Cert, kSecItemPemArmour, NULL, &data); - if (err != noErr) { - continue; - } - if (data != NULL) { - CFDataAppendBytes(appendTo, CFDataGetBytePtr(data), CFDataGetLength(data)); - CFRelease(data); - } - } - CFRelease(certs); - } - *pemRoots = combinedData; - *untrustedPemRoots = combinedUntrustedData; - return 0; -} -*/ -import "C" -import ( - "errors" - "unsafe" -) - -func init() { - loadSystemRootsWithCgo = _loadSystemRootsWithCgo -} - -func _loadSystemRootsWithCgo() (*CertPool, error) { - var data, untrustedData C.CFDataRef - err := C.CopyPEMRoots(&data, &untrustedData, C.bool(debugDarwinRoots)) - if err == -1 { - return nil, errors.New("crypto/x509: failed to load darwin system roots with cgo") - } - defer C.CFRelease(C.CFTypeRef(data)) - defer C.CFRelease(C.CFTypeRef(untrustedData)) - - buf := C.GoBytes(unsafe.Pointer(C.CFDataGetBytePtr(data)), C.int(C.CFDataGetLength(data))) - roots := NewCertPool() - roots.AppendCertsFromPEM(buf) - - if C.CFDataGetLength(untrustedData) == 0 { - return roots, nil - } - - buf = C.GoBytes(unsafe.Pointer(C.CFDataGetBytePtr(untrustedData)), C.int(C.CFDataGetLength(untrustedData))) - untrustedRoots := NewCertPool() - untrustedRoots.AppendCertsFromPEM(buf) - - trustedRoots := NewCertPool() - for _, lc := range roots.lazyCerts { - c, err := lc.getCert() - if err != nil { - return nil, err - } - if !untrustedRoots.contains(c) { - trustedRoots.AddCert(c) - } - } - return trustedRoots, nil -} diff --git a/src/crypto/x509/root_darwin.go b/src/crypto/x509/root_darwin.go index ce88de025e..c9ea7e80f3 100644 --- a/src/crypto/x509/root_darwin.go +++ b/src/crypto/x509/root_darwin.go @@ -20,10 +20,6 @@ func (c *Certificate) systemVerify(opts *VerifyOptions) (chains [][]*Certificate return nil, nil } -// loadSystemRootsWithCgo is set in root_cgo_darwin_amd64.go when cgo is -// available, and is only used for testing. -var loadSystemRootsWithCgo func() (*CertPool, error) - func loadSystemRoots() (*CertPool, error) { var trustedRoots []*Certificate untrustedRoots := make(map[string]bool) diff --git a/src/crypto/x509/root_darwin_test.go b/src/crypto/x509/root_darwin_test.go index 69f181c2d4..ae2bd02bf8 100644 --- a/src/crypto/x509/root_darwin_test.go +++ b/src/crypto/x509/root_darwin_test.go @@ -28,39 +28,6 @@ func TestSystemRoots(t *testing.T) { t.Errorf("want at least %d system roots, have %d", want, have) } - if loadSystemRootsWithCgo == nil { - t.Skip("cgo not available, can't compare pool") - } - - t1 := time.Now() - cgoRoots, err := loadSystemRootsWithCgo() // cgo roots - cgoSysRootsDuration := time.Since(t1) - - if err != nil { - t.Fatalf("failed to read cgo roots: %v", err) - } - - t.Logf("loadSystemRootsWithCgo: %v", cgoSysRootsDuration) - - // Check that the two cert pools are the same. - sysPool := make(map[string]*Certificate, sysRoots.len()) - for i := 0; i < sysRoots.len(); i++ { - c := sysRoots.mustCert(t, i) - sysPool[string(c.Raw)] = c - } - for i := 0; i < cgoRoots.len(); i++ { - c := cgoRoots.mustCert(t, i) - - if _, ok := sysPool[string(c.Raw)]; ok { - delete(sysPool, string(c.Raw)) - } else { - t.Errorf("certificate only present in cgo pool: %v", c.Subject) - } - } - for _, c := range sysPool { - t.Errorf("certificate only present in real pool: %v", c.Subject) - } - if t.Failed() { cmd := exec.Command("security", "dump-trust-settings") cmd.Stdout, cmd.Stderr = os.Stderr, os.Stderr diff --git a/src/crypto/x509/root_ios.go b/src/crypto/x509/root_ios.go index bb4a5f75ba..cb3529d6d5 100644 --- a/src/crypto/x509/root_ios.go +++ b/src/crypto/x509/root_ios.go @@ -10,9 +10,6 @@ func (c *Certificate) systemVerify(opts *VerifyOptions) (chains [][]*Certificate return nil, nil } -// loadSystemRootsWithCgo is not available on iOS. -var loadSystemRootsWithCgo func() (*CertPool, error) - func loadSystemRoots() (*CertPool, error) { p := NewCertPool() p.AppendCertsFromPEM([]byte(systemRootsPEM)) diff --git a/src/crypto/x509/root_ios_gen.go b/src/crypto/x509/root_ios_gen.go index f7eecb576d..2bcdab1a77 100644 --- a/src/crypto/x509/root_ios_gen.go +++ b/src/crypto/x509/root_ios_gen.go @@ -172,9 +172,6 @@ func (c *Certificate) systemVerify(opts *VerifyOptions) (chains [][]*Certificate return nil, nil } -// loadSystemRootsWithCgo is not available on iOS. -var loadSystemRootsWithCgo func() (*CertPool, error) - func loadSystemRoots() (*CertPool, error) { p := NewCertPool() p.AppendCertsFromPEM([]byte(systemRootsPEM)) diff --git a/src/crypto/x509/root_omit.go b/src/crypto/x509/root_omit.go index 175d71643b..0055b3b862 100644 --- a/src/crypto/x509/root_omit.go +++ b/src/crypto/x509/root_omit.go @@ -24,6 +24,3 @@ func loadSystemRoots() (*CertPool, error) { func (c *Certificate) systemVerify(opts *VerifyOptions) (chains [][]*Certificate, err error) { return nil, nil } - -// loadSystemRootsWithCgo is not available on iOS. -var loadSystemRootsWithCgo func() (*CertPool, error) diff --git a/src/go/build/deps_test.go b/src/go/build/deps_test.go index b26b2bd199..bf1367355d 100644 --- a/src/go/build/deps_test.go +++ b/src/go/build/deps_test.go @@ -395,7 +395,7 @@ var depsRules = ` CGO, net !< CRYPTO-MATH; # TLS, Prince of Dependencies. - CGO, CRYPTO-MATH, NET, container/list, encoding/hex, encoding/pem + CRYPTO-MATH, NET, container/list, encoding/hex, encoding/pem < golang.org/x/crypto/internal/subtle < golang.org/x/crypto/chacha20 < golang.org/x/crypto/poly1305