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: build tag "ios" broken at tip on darwin/amd64 #38710

Closed
bradfitz opened this issue Apr 27, 2020 · 15 comments
Closed

crypto/x509: build tag "ios" broken at tip on darwin/amd64 #38710

bradfitz opened this issue Apr 27, 2020 · 15 comments
Labels
FrozenDueToAge mobile Android, iOS, and x/mobile NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone

Comments

@bradfitz
Copy link
Contributor

In previous Go releases, the "ios" build tag with darwin/amd64 produced working binaries:

barloga5k:x509 $ go1.14.2 download
...
Downloaded 100.0% (125040726 / 125040726 bytes)
Unpacking /Users/bradfitz/sdk/go1.14.2/go1.14.2.darwin-amd64.tar.gz ...
Success. You may now run 'go1.14.2'
barloga5k:x509 $ GOARCH=amd64 go1.14.2 install --tags=ios crypto/x509
barloga5k:x509 $

(Such a configuration is used when running under the iOS simulator in Xcode)

But at Go tip:

barloga5k:x509 $ go version
go version devel +a7e9e84716 Mon Apr 27 20:20:53 2020 +0000 darwin/amd64
barloga5k:x509 $ go install --tags=ios crypto/x509
# crypto/x509
./cert_pool.go:65:9: undefined: loadSystemRoots
./root.go:21:32: undefined: loadSystemRoots

Looks like https://go-review.googlesource.com/c/go/+/227197 (da8591b, @aclements) dropped the // +build arm arm64 ios disjunction.

Labeling release-blocker in that it's a regression.

@bradfitz bradfitz added NeedsFix The path to resolution is known, but the work has not been done. release-blocker mobile Android, iOS, and x/mobile labels Apr 27, 2020
@bradfitz bradfitz added this to the Go1.15 milestone Apr 27, 2020
bradfitz added a commit to tailscale/go that referenced this issue Apr 27, 2020
See golang#38710

This fixes our iOS simulator build.
@FiloSottile
Copy link
Contributor

The ios build tag was only used in crypto/x509, and not documented anywhere I could find. Is "like iOS but on amd64" a supported configuration? Is crypto/x509 really the only thing that needs to support it?

@bradfitz
Copy link
Contributor Author

I have no clue about the history other than it used to work and I stumbled into a project that was depending on it.

@bradfitz
Copy link
Contributor Author

(fwiw, I was also surprised it ever worked)

@aclements
Copy link
Member

Probably we just need to drop the !ios build constraint from root_cgo_darwin.go? I guess previously that was meant to be exclusive with root_darwin_armx.go, but I only dropped the positive build tag. That would just make it ignore the ios build tag.

@bradfitz
Copy link
Contributor Author

@aclements, that might not work because cgo might be disabled in such an environment.

What I ended up doing for our build was renaming those files from "arm" to "embed" (in tailscale@1f0b2d1) and making the build tags be:

// +build !x509omitbundledroots
// +build darwin
// +build arm64 ios

@aclements
Copy link
Member

@FiloSottile , what do you think is the right fix here? It should be easy to fix, I'm just not sure what the right way to do it is. :)

@dmitshur dmitshur added the okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1 label May 14, 2020
@toothrot toothrot removed the okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1 label Jun 10, 2020
@toothrot
Copy link
Contributor

@FiloSottile Friendly ping as this is a Go 1.15 release blocking issue.

@dmitshur
Copy link
Contributor

I have no clue about the history other than it used to work and I stumbled into a project that was depending on it.

@bradfitz Can you elaborate on how the project is depending on it? What was the expected behavior? If we can make it more clear what the mode represents and is supposed to do, it will make it easier to make progress here. I'll update the issue state to NeedsInvestigation because I don't think we know what the fix needs to be yet (as confirmed from discussion above).

While it's true this is a regression, if we take the potential plan for 1.15 described in #38485 (comment) into account, we may want to deliberately make a change here anyway. This would be in scope of Go 1 compatibility promise since the mobile support is considered experimental and the ios tag was not documented. I don't know yet if we'll want or need to do that, just pointing out that issue is relevant here.

I understand the current (Go 1.14) modes of operation and the tags that are satisfied are:

Mode Satisfied Build Tags
macOS, x86 64-bit darwin, amd64, !ios
iOS mobile, ARM 64-bit darwin, arm64, !ios
Issue #38710 mode darwin, amd64, ios

We would execute the plan in #38485 (comment) only if we know that darwin/arm64 needs to be reclaimed for macOS, which would be if there is an announcement of a future ARM-based macOS configuration (we may learn more from WWDC 2020 keynote in 4~ days). So the forward-looking table may look like this:

Mode Satisfied Build Tags
macOS, x86 64-bit darwin, amd64, !ios
macOS, ARM 64-bit darwin, arm64, !ios
iOS mobile, ARM 64-bit darwin, arm64, ios
Issue #38710 mode darwin, amd64, ios

At that point, it would only be possible to preserve "Issue #38710 mode" behavior of Go 1.14 on an incomplete subset of macOS configurations. If it's important for this mode to be available to all macOS users, some change will be needed.

@bradfitz From the commit message of tailscale@1f0b2d1, I understand "Issue #38710 mode" is related to the iOS simulator. Do you know if it needs to be different from normal iOS mobile (other than the architecture difference)?

@dmitshur dmitshur added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. and removed NeedsFix The path to resolution is known, but the work has not been done. labels Jun 17, 2020
@dmitshur
Copy link
Contributor

dmitshur commented Jun 22, 2020

I've looked into the history and understand this better now.

The ios build tag was only used in crypto/x509, and not documented anywhere I could find. Is "like iOS but on amd64" a supported configuration?

I found some documentation for it in Go 1.5 release notes:

On Darwin, the use of the system X.509 certificate interface can be disabled
with the ios build tag.

There's more background in CL 12301 that added it and issue #11736 that it fixed (/cc @crawshaw).

So as I understand it now, "Issue #38710 mode" (in the tables of my previous comment) is indistinguishable from "iOS mobile" mode: it just means to disable the code to fetch system roots via API calls and use the embedded ones instead.

So, for 1.15 specifically, this shouldn't intersect with #38485, because the described behavior can be applied for both amd64 and arm64 architectures.

I see there has been a new x509omitbundledroots build tag added in CL 229762 (/cc @bradfitz) for 1.15, but it should work the same for darwin,amd64,ios and darwin,arm64,ios, so I'm not seeing a conflict.

Is crypto/x509 really the only thing that needs to support it?

@hyangah's comment in #38485 (comment) also suggested that the ios tag is only used in crypto/x509 package.

@dmitshur
Copy link
Contributor

The build failure can't be reproduced on tip anymore.

It turns out it was fixed during Cthulhu's great awakening in CL 227037 (/cc @FiloSottile), sadly missing out on a chance to add another "Fixes" line:

x509 $ git co 6f52790a20a2432ae61e0ec9852a3df823a16d40^
HEAD is now at 6ea19bb668 crypto/tls: rotate session keys in older TLS versions
x509 $ go build -tags=ios                              
# crypto/x509
./cert_pool.go:65:9: undefined: loadSystemRoots
./root.go:21:32: undefined: loadSystemRoots
x509 $ git co 6f52790a20a2432ae61e0ec9852a3df823a16d40 
Previous HEAD position was 6ea19bb668 crypto/tls: rotate session keys in older TLS versions
HEAD is now at 6f52790a20 crypto/x509: use Security.framework without cgo for roots on macOS
x509 $ go build -tags=ios                             
x509 $ echo $?
0

I'll look into if the right files are selected, and maybe send a test that checks that for all relevant build configurations, so we have more confidence and this doesn't regress.

Filippo has offered to look at this issue this week, so I'm counting on him to look things over afterwards.

@dmitshur
Copy link
Contributor

dmitshur commented Jun 23, 2020

I'll look into if the right files are selected, and maybe send a test that checks that for all relevant build configurations, so we have more confidence and this doesn't regress.

I'm not spotting an issue with the selected files from a first look.

@FiloSottile, does the aforementioned test that checks various build configurations and the .go files that are selected sound worth including? A draft looks like this:


// Test supported build modes. Primarily to provide coverage for
// mobile platforms (Android, iOS) and special build tags.
func TestBuildModes(t *testing.T) {
	if testing.Short() {
		t.Skip("skipping in -short mode")
	}

	tests := []struct {
		name         string
		goos, goarch string
		cgo          bool
		tags         []string
		wantGoFiles  string // Space-separated list of all .go files selected in the build.
	}{
		{
			"Linux (linux/ppc64), Cgo on",
			"linux", "ppc64", true, nil,
			"cert_pool.go pem_decrypt.go pkcs1.go pkcs8.go root.go root_linux.go root_unix.go sec1.go verify.go x509.go",
		},
		{
			"Android (android/arm64), Cgo on",
			"android", "arm64", true, nil,
			"cert_pool.go pem_decrypt.go pkcs1.go pkcs8.go root.go root_linux.go root_unix.go sec1.go verify.go x509.go",
		},

		{
			"macOS Intel-based 64-bit (darwin/amd64), Cgo off",
			"darwin", "amd64", false, nil,
			"cert_pool.go pem_decrypt.go pkcs1.go pkcs8.go root.go root_darwin_amd64.go sec1.go verify.go x509.go",
		},
		{
			"macOS Intel-based 64-bit (darwin/amd64), Cgo on",
			"darwin", "amd64", true, nil,
			"cert_pool.go pem_decrypt.go pkcs1.go pkcs8.go root.go root_darwin_amd64.go sec1.go verify.go x509.go root_cgo_darwin_amd64.go",
		},

		{
			"iOS 64-bit (darwin/arm64, ios build tag), Cgo on",
			"darwin", "arm64", true, []string{"ios"},
			"cert_pool.go pem_decrypt.go pkcs1.go pkcs8.go root.go root_darwin_ios.go sec1.go verify.go x509.go",
		},
		{
			"iOS 64-bit (darwin/arm64, ios build tag), Cgo on, x509omitbundledroots build tag",
			"darwin", "arm64", true, []string{"ios", "x509omitbundledroots"},
			"cert_pool.go pem_decrypt.go pkcs1.go pkcs8.go root.go root_omit.go sec1.go verify.go x509.go",
		},
		{
			"iOS 64-bit in iOS simulator on Intel-based macOS (darwin/amd64, ios build tag), Cgo on",
			"darwin", "amd64", true, []string{"ios"},
			"cert_pool.go pem_decrypt.go pkcs1.go pkcs8.go root.go root_darwin_ios.go sec1.go verify.go x509.go",
		},
		{
			"iOS 64-bit in iOS simulator on Intel-based macOS (darwin/amd64, ios build tag), Cgo on, x509omitbundledroots build tag",
			"darwin", "amd64", true, []string{"ios", "x509omitbundledroots"},
			"cert_pool.go pem_decrypt.go pkcs1.go pkcs8.go root.go root_omit.go sec1.go verify.go x509.go",
		},

		// These modes are not supported in Go 1.15, but checked just
		// to avoid making a larger change than needed for issue 38710.
		{
			"unsupported mode close to iOS 64-bit", // Just ensure it's the same as iOS 64-bit for 1.15.
			"darwin", "arm64", true, []string{},
			"cert_pool.go pem_decrypt.go pkcs1.go pkcs8.go root.go root_darwin_ios.go sec1.go verify.go x509.go",
		},
		{
			"unsupported mode close to iOS 64-bit, x509omitbundledroots build tag", // Just ensure it's the same as iOS 64-bit for 1.15.
			"darwin", "arm64", true, []string{"x509omitbundledroots"},
			"cert_pool.go pem_decrypt.go pkcs1.go pkcs8.go root.go root_omit.go sec1.go verify.go x509.go",
		},
	}

	for _, test := range tests {
		bctx := build.Default
		bctx.GOOS, bctx.GOARCH = test.goos, test.goarch
		bctx.CgoEnabled = test.cgo
		bctx.BuildTags = test.tags

		p, err := bctx.Import("crypto/x509", "", 0)
		if err != nil {
			t.Fatal(err)
		}
		if len(p.InvalidGoFiles) != 0 {
			t.Errorf("%s: InvalidGoFiles = %q, want none", test.name, p.InvalidGoFiles)
		}
		if got, want := strings.Join(append(p.GoFiles, p.CgoFiles...), " "), test.wantGoFiles; got != want {
			t.Errorf("%s:\nsource files = %q\n        want = %q", test.name, got, want)
		}
	}
}

(It doesn't include all build configurations, only the ones that seem at risk because they're in x/mobile and might not get easily caught.)

@FiloSottile
Copy link
Contributor

I am not sure this is fixed, partially because I am still not sure what this is supposed to do, besides "compile".

In Go 1.14 the ios tag makes the package use the bundled roots even if on amd64. CL 227197 broke the build because it dropped the ios tag without dropping the !ios tag. CL 227037 fixed the build because it also dropped the !ios tag.

The behavior though is now not the same as Go 1.14: with the (now meaningless) ios tag, the package will now still use Security.framework rather than the embedded roots. The problem is that I don't know if Security.framework is acceptable in the iOS simulator (while maybe neither cgo nor shelling out were), or if we need to reintroduce the tag. There's also a chance we'll have to patch the runtime because I think it now unconditionally links Security.framework.

A test that just hardcodes the selected files seems brittle. At least, we should check that the package builds, like for x509omitbundledroots, but even better would be to figure out if we can run the iOS simulator, if that's something we want to support.

@bradfitz is there a way to run the simulator from the command line?

@dmitshur
Copy link
Contributor

dmitshur commented Jun 23, 2020

I'm not spotting an issue with the selected files from a first look.

I am spotting an issue now. As you mentioned, it's not using embedded roots in iOS mode (only when GOARCH is amd64; not when it's arm64). Sorry for a misleading comment.

The problem is that I don't know if Security.framework is acceptable in the iOS simulator (while maybe neither cgo nor shelling out were), or if we need to reintroduce the tag.

I think keeping the same behavior for iOS as it was in 1.14 would be the safest path forward, which means it should continue to use embedded roots (optionally compressed/lazy loaded if x509omitbundledroots build tag is also specified).

Upgrading iOS from using embedded roots to fetching them from via APIs, if it happens to be possible now via Security.framework, can be a feature request to be investigated for 1.16 or later. (It would also make the x509omitbundledroots build tag obsolete/no-op.)

I am still not sure what this is supposed to do, besides "compile".

Is the fourth paragraph in #38710 (comment) helpful? In iOS mode (regardless if the GOARCH used is arm64 or amd64), embedded roots should be used.

is there a way to run the simulator from the command line?

As far as I know, the only officially supported way will need to use x/mobile's gomobile tool, and it will need an Apple developer identity to be available in the environment. I don't think that a test that can be added to crypto/x509. Adding a new builder is a reasonable request, but seems out of scope for this issue.

@dmitshur dmitshur removed their assignment Jun 23, 2020
@dmitshur
Copy link
Contributor

I am spotting an issue now. As you mentioned, it's not using embedded roots in iOS mode (only when GOARCH is amd64; not when it's arm64).

The following test cases, which I forgot to include earlier, catch this:

{
	"iOS 64-bit in iOS simulator on Intel-based macOS (darwin/amd64, ios build tag), Cgo on",
	"darwin", "amd64", true, []string{"ios"},
	"cert_pool.go pem_decrypt.go pkcs1.go pkcs8.go root.go root_darwin_arm64.go sec1.go verify.go x509.go",
},
{
	"iOS 64-bit in iOS simulator on Intel-based macOS (darwin/amd64, ios build tag), Cgo on, x509omitbundledroots build tag",
	"darwin", "amd64", true, []string{"ios", "x509omitbundledroots"},
	"cert_pool.go pem_decrypt.go pkcs1.go pkcs8.go root.go root_omit.go sec1.go verify.go x509.go",
},
$ go test -v -run=TestBuildModes crypto/x509
=== RUN   TestBuildModes
    x509_test.go:1464: iOS 64-bit in iOS simulator on Intel-based macOS (darwin/amd64, ios build tag), Cgo on: source files = "cert_pool.go pem_decrypt.go pkcs1.go pkcs8.go root.go root_darwin_amd64.go sec1.go verify.go x509.go root_cgo_darwin_amd64.go", want "cert_pool.go pem_decrypt.go pkcs1.go pkcs8.go root.go root_darwin_arm64.go sec1.go verify.go x509.go"
    x509_test.go:1464: iOS 64-bit in iOS simulator on Intel-based macOS (darwin/amd64, ios build tag), Cgo on, x509omitbundledroots build tag: source files = "cert_pool.go pem_decrypt.go pkcs1.go pkcs8.go root.go root_darwin_amd64.go sec1.go verify.go x509.go root_cgo_darwin_amd64.go", want "cert_pool.go pem_decrypt.go pkcs1.go pkcs8.go root.go root_omit.go sec1.go verify.go x509.go"
--- FAIL: TestBuildModes (0.01s)
FAIL
FAIL	crypto/x509	0.394s
FAIL

@gopherbot
Copy link

Change https://golang.org/cl/239559 mentions this issue: crypto/x509: restore support for ios tag on darwin/amd64

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jun 24, 2020
@golang golang locked and limited conversation to collaborators Jun 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge mobile Android, iOS, and x/mobile NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Development

No branches or pull requests

6 participants