Use syscall package instead of cgo

[imjasonh]

The use of cgo was introduced in #268, instead of using golang.org/x/sys/unix. I believe syscall.Setres{g,u}id is the platform-independent equivalent, but I admit I don't know enough about this to be confident this isn't a regression.

If this is acceptable, it could be a step toward unblocking #435

@ekcasey

[micahyoung]

I personally really like this if it works equivalently. In addition to opening up arm, it would even simplify building for amd64, which currently when building with make build-linux, uses a Docker container (Makefile).

I did a quick search and it appears there was a Golang bug related to multithreading in Setuid which appears to have been fixed and merged into 1.16 in Dec 2020. We may need to bump lifecycle's Golang to 1.16 (and related build images, etc).

But we'll see what Emily says. In the meantime, I triggered a build and it is looking green.

[imjasonh]

Thanks @micahyoung! I'm hopeful. 🤞

We may need to bump lifecycle's Golang to 1.16 (and related build images, etc).

Incidentally, I'm also bumping to 1.16 in #628 for unrelated reasons.

[natalieparellano]

IIUC the validation for this would involve running a phase like the analyzer a bunch of times as root and then observing if all the threads have the correct user when we drop privileges. @ekcasey I recall you did this validation ages ago, can you remember how many "trials" were sufficient to demonstrate that it was working?

[natalieparellano]

After speaking with @ekcasey it seems that previously, when we tried to use the syscall package to drop privileges, the issue manifested as failed builds, because files or directories were getting written as the wrong user. To test this, I added the following test to analyzer acceptance:

		it("drops privileges", func() {
			h.SkipIf(t, runtime.GOOS == "windows", "Not relevant on Windows")

			output := h.DockerRun(t,
				analyzeImage,
				h.WithFlags(
					"--network", registryNetwork,
					"--env", "CNB_PLATFORM_API="+platformAPI,
				),
				h.WithBash(
					fmt.Sprintf("%s -analyzed /some-dir/some-analyzed.toml %s; ls -al /some-dir",
						ctrPath(analyzerPath),
						noAuthRegistry.RepoName("some-image"),
					),
				),
			)

			h.AssertMatch(t, output, "2222 3333 .+ some-analyzed.toml")
		})

...and it fails (non-deterministically) when applied to this branch 😭 (but passes on main)

    analyzer_test.go:281: Expected: 'Warning: Not restoring cached layer metadata, no cache flag specified.
        Previous image with name "192.168.1.43:49946/some-image" not found
        total 12
        drwxr-xr-x 1 2222 3333 4096 Jul  8 20:26 .
        drwxr-xr-x 1 root root 4096 Jul  8 20:26 ..
        -rw-r--r-- 1 2222 3333    0 Mar 26 15:37 .gitkeep
        -rw-r--r-- 1 root root  247 Jul  8 20:26 some-analyzed.toml
        ' to match regex '2222 3333 .+ some-analyzed.toml'

I'm noticing that this branch still uses go 1.15, so maybe rebasing onto main (with 1.16) will help. I put up a PR to add the mentioned test, hopefully when that is merged it will make this one easier to test.

[codecov]

Codecov Report

Merging #630 (4b9a186) into main (979103d) will increase coverage by 1.56%.
The diff coverage is n/a.

❗ Current head 4b9a186 differs from pull request most recent head 15eaa2b. Consider uploading reports for the commit 15eaa2b to get more accurate results

@@            Coverage Diff             @@
##             main     #630      +/-   ##
==========================================
+ Coverage   64.92%   66.47%   +1.56%     
==========================================
  Files          52       59       +7     
  Lines        3634     3829     +195     
==========================================
+ Hits         2359     2545     +186     
+ Misses       1026     1018       -8     
- Partials      249      266      +17     

Flag	Coverage Δ
os_linux	68.25% <ø> (?)
os_windows	64.90% <ø> (-0.02%)	⬇️
unit	68.25% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

[imjasonh]

I'm noticing that this branch still uses go 1.15, so maybe rebasing onto main (with 1.16) will help. I put up a PR to add the mentioned test, hopefully when that is merged it will make this one easier to test.

#628 updated CI to use Go 1.15 instead, I've rebased this PR on main to pick this up 🤞

Thanks for adding the acceptance test! 👍

[natalieparellano]

Looks like it's working! I've had 20+ green tests, where before the failure rate was approximately 4 out of 5 🎉