feat(sbom): add a directive to generate SBOM for a layer #420

rchincha · 2023-02-27T02:20:24Z

What type of PR is this?

Which issue does this PR fix:

What does this PR do / Why do we need it:

If an issue # is not available please add repro steps and logs showing the issue:

Testing done on this change:

Automation added to e2e:

Will this break upgrades or downgrades?

Does this PR introduce any user-facing change?:

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

pkg/container/container.go

pkg/stacker/bom.go

rchincha · 2023-08-14T17:44:28Z

2023-08-14T17:31:17.2117593Z # bind mounting /home/runner/work/stacker/stacker/stackertest-test_can_read_previous_version-27s_cache.WKvnV5/.stacker/imports/test into container
2023-08-14T17:31:17.2118033Z # + cp /stacker/imports/foo /foo
2023-08-14T17:31:17.2118442Z # cp: cannot stat '/stacker/imports/foo': No such file or directory

Since we have a breaking change in this PR

smoser

Some comments inline.
Thank you for your work on this.

smoser · 2023-08-15T11:13:02Z

cmd/stacker/internal_go.go

+		return errors.Errorf("wrong number of args")
+	}
+
+	// merge


this is a strange comment here. I guess maybe it would make sense down a couple lines. Just reading it I thought "wait, this is Verify, not merge".

smoser · 2023-08-15T11:16:18Z

Makefile

@@ -45,7 +45,6 @@ stacker-dynamic: $(STACKER_DEPS) cmd/stacker/lxc-wrapper/lxc-wrapper
 cmd/stacker/lxc-wrapper/lxc-wrapper: cmd/stacker/lxc-wrapper/lxc-wrapper.c
 	make -C cmd/stacker/lxc-wrapper LDFLAGS=-static LDLIBS="$(shell pkg-config --static --libs lxc) -lpthread -ldl" lxc-wrapper

-


just un-touch this file

smoser · 2023-08-15T11:22:43Z

cmd/stacker/internal_go.go

+	return nil
+}
+
+// pkgname, license, paths...


improve or remove this comment. It doesn't line up with the Args().Get calls below.

smoser · 2023-08-15T11:24:14Z

cmd/stacker/internal_go.go

+	// check against inventory
+	if err := fs.GenerateInventory("/",
+		[]string{"/proc", "/sys", "/dev", "/etc/resolv.conf",
+			"/stacker", "/stacker/artifacts", "/stacker-bom", "/static-stacker"},


i think you can drop /stacker-bom now ? and /stactic-stacker ?
this should just have '/stacker/' right?

smoser · 2023-08-15T11:32:18Z

pkg/stacker/publisher.go

+}
+
+// publishArtifact to a registry/repo for this subject
+func (p *Publisher) publishArtifact(path, mtype, registry, repo, subject string, skipTLS bool) error {


Are there 'containers/image' library calls that you could use?

smoser · 2023-08-15T11:36:43Z

pkg/stacker/build.go

@@ -476,7 +476,7 @@ func (b *Builder) build(s types.Storage, file string) error {
 			}

 			// These should all be non-interactive; let's ensure that.
-			err = c.Execute("/stacker/.stacker-run.sh", nil)
+			err = c.Execute("/stacker/imports/.stacker-run.sh", nil)


this feels weird in /stacker/imports, but it seems unlikely to stomp on user content. so unless you think of something that fits better, this seems fine.

Maybe we can put this under /stacker/tools/.stacker-run.sh

Actually, this is under "automatic" binding of /stacker/imports, let's not change that.

smoser · 2023-08-15T11:49:30Z

test/bom.bats

+        /etc/sysconfig/sshd-permitrootlogin /root/anaconda-* /root/original-* /run/nologin \
+        /var/lib/rpm/.rpm.lock /etc/.pwd.lock /etc/BUILDTIME
+    annotations:
+      org.opencontainers.image.authors: bom-test


I'm curious if whitespace correctly goes through into the internal command.
Does it work if your 'authors' are "George Williams, Mary Andrews".

Same for 'vendor' being "ACME Widgets & Trinkets Inc".

Let's amend test and try.

test_bom_tool_should_work_inside_run.wZP2W7/oci --roots-dir /home/runner/work/stacker/stacker/stackertest-test_bom_tool_should_work_inside_run.wZP2W7/roots --stacker-dir /home/runner/work/stacker/stacker/stackertest-test_bom_tool_should_work_inside_run.wZP2W7/.stacker --storage-type overlay --internal-userns --debug internal-go bom-build /stacker/artifacts George Williams, Mary Andrews ACME Widgets & Trinkets Inc. MIT pkg1 1.0.0 /pkg1]

breaks!

smoser · 2023-08-15T11:53:49Z

pkg/stacker/build.go

+			return errors.Wrapf(err, "couldn't find executable for bind mount")
+		}
+
+		err = c.BindMount(binary, "/stacker/tools/static-stacker", "")


Is there a reason to call this 'static-stacker' rather than just 'stacker' ?

Just keeping with existing convention and also run-on /stacker/tools/stacker?

i think that makes sense.

smoser · 2023-08-15T11:54:34Z

pkg/stacker/bom.go

+		return err
+	}
+
+	cmd := fmt.Sprintf("/static-stacker --oci-dir %s --roots-dir %s --stacker-dir %s --storage-type %s --internal-userns",


Is it possible to change this '/static-stacker' into /stacker/tools/ for consistency?

smoser · 2023-08-16T14:02:16Z

pkg/stacker/build.go

+			return errors.Wrapf(err, "couldn't find executable for bind mount")
+		}
+
+		err = c.BindMount(binary, "/stacker/tools/static-stacker", "")


i think that makes sense.

smoser · 2023-08-16T14:13:21Z

test/bom.bats

+        paths: [/pkg2]
+    run: |
+      # discover installed pkgs
+      /stacker/tools/static-stacker --internal-userns internal-go bom-discover


The mechanism that you had before for executing stacker-bom was much cleaner. we should not expect a user to use '--internal-userns internal-go' flags (we should never document 'internal-go' flags).

We want the documented command that the user runs to be one of:

stacker bom-discover

stacker-bom discover

stacker bom discover

I think i prefer 3, and making 'stacker bom discover' just work wherever it was run.

I think the right way to fix that is to add the bom commands to main (not internal-go), and then make shouldSkipInternalUserns (cmd/stacker/main.go) return false for 'bom' commands. stacker bom subcommand never needs to enter a internal user namespace. It just inspects directory trees and writes content somewhere.

smoser · 2023-08-16T14:18:14Z

cmd/stacker/internal_go.go

+		},
+		&cli.Command{
+			Name:   "bom-verify",
+			Action: doBomVerify,


We don't have any convention in stacker here yet as to whether we want:

sub-subcommands (stacker bom build) or hyphenated commands (stacker bom-build).

because we're going to expose that and even document it to the user as the way to generate bom information, we should at least think on it.

smoser · 2023-08-17T15:26:13Z

pkg/stacker/bom.go

+	org := l.Annotations[types.OrgAnnotation]
+	license := l.Annotations[types.LicenseAnnotation]
+	dest := "/stacker/artifacts"
+	cmd := append(stackerInternal, "bom-build", dest, author, org, license, pkg.Name, pkg.Version)


it seems wrong to me that we pass the pkg.Name, pkg.Version (which are in the 'Package' type), but do not pass the pkg.License. Rather, we pass the 'license' that came from the globally scoped 'license' from the LicenseAnnotation.

The Package explicitly had a license and we took the one from the larger scope.

should the Package type have a Author and Org on it and then you use the higher-scoped annotations if they are not present?

I guess i would not have expected the 'author', 'org' at the global level to apply all of the Packages.

A bit tricky - yes we should have license for each pkg entry. However, the global license is what license the container image itself is being posted under.

A new bind-mount /stacker-artifacts is added to a container into which all artifacts including sbom can be added. Once the container image is built, then in the publish phase we push sbom along with the image as a OCI dist-spec "reference". Signed-off-by: Ramkumar Chinchani <rchincha@cisco.com>

Signed-off-by: Ramkumar Chinchani <rchincha@cisco.com>

BREAKING CHANGE: Some paths per earlier stacker conventions are now changed as follows. /stacker/imports : ro mount for imports /stacker/artifacts : rw mount to store output of next step /stacker/tools : /proc/self mounted as /stacker/tools/bom /stacker/oci-labels : where OCI label generation logic now resides NOTE: Making this a separate commit if we want to revert Signed-off-by: Ramkumar Chinchani <rchincha@cisco.com>

Signed-off-by: Ramkumar Chinchani <rchincha@cisco.com>

rchincha force-pushed the sbom branch 24 times, most recently from 7da4f20 to f03e0b2 Compare March 23, 2023 20:36

smoser reviewed Mar 24, 2023

View reviewed changes

pkg/container/container.go Outdated Show resolved Hide resolved

smoser reviewed Mar 24, 2023

View reviewed changes

pkg/stacker/bom.go Outdated Show resolved Hide resolved

rchincha force-pushed the sbom branch 4 times, most recently from 26ed290 to 80e499c Compare March 27, 2023 21:44

rchincha force-pushed the sbom branch 2 times, most recently from 337145c to d0cca1a Compare August 14, 2023 16:54

rchincha force-pushed the sbom branch 2 times, most recently from 203801c to cd0f492 Compare August 14, 2023 20:59

smoser reviewed Aug 15, 2023

View reviewed changes

rchincha force-pushed the sbom branch 6 times, most recently from 762cfb4 to 63b13f0 Compare August 15, 2023 23:00

smoser reviewed Aug 16, 2023

View reviewed changes

rchincha force-pushed the sbom branch 4 times, most recently from fc9b711 to c9f0ef0 Compare August 17, 2023 07:30

smoser reviewed Aug 17, 2023

View reviewed changes

rchincha force-pushed the sbom branch 3 times, most recently from 5737a56 to f526245 Compare August 18, 2023 18:18

rchincha added 5 commits August 18, 2023 20:50

feat(publish): add support for publishing OCI artifacts/references

8c79c34

Signed-off-by: Ramkumar Chinchani <rchincha@cisco.com>

fix: allow importing a parent sbom

1e90ef9

Signed-off-by: Ramkumar Chinchani <rchincha@cisco.com>

test: don't keep bom if failure

98a1fb9

Signed-off-by: Ramkumar Chinchani <rchincha@cisco.com>

rchincha force-pushed the sbom branch from 247462e to e28dec4 Compare August 18, 2023 20:50

fix: address PR comments

a5ee2b2

Signed-off-by: Ramkumar Chinchani <rchincha@cisco.com>

rchincha force-pushed the sbom branch from e28dec4 to a5ee2b2 Compare August 18, 2023 21:12

rchincha merged commit c68147c into project-stacker:main Aug 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(sbom): add a directive to generate SBOM for a layer #420

feat(sbom): add a directive to generate SBOM for a layer #420

rchincha commented Feb 27, 2023

rchincha commented Aug 14, 2023

smoser left a comment

smoser Aug 15, 2023

rchincha Aug 15, 2023

smoser Aug 15, 2023

rchincha Aug 15, 2023

smoser Aug 15, 2023

rchincha Aug 15, 2023

smoser Aug 15, 2023

smoser Aug 15, 2023

rchincha Aug 15, 2023

smoser Aug 15, 2023

rchincha Aug 15, 2023

rchincha Aug 15, 2023

smoser Aug 15, 2023

rchincha Aug 15, 2023

rchincha Aug 15, 2023

smoser Aug 15, 2023

rchincha Aug 15, 2023

smoser Aug 16, 2023

smoser Aug 15, 2023

rchincha Aug 15, 2023

smoser Aug 16, 2023

smoser Aug 16, 2023

smoser Aug 16, 2023

smoser Aug 17, 2023

rchincha Aug 17, 2023

		@@ -45,7 +45,6 @@ stacker-dynamic: $(STACKER_DEPS) cmd/stacker/lxc-wrapper/lxc-wrapper
		cmd/stacker/lxc-wrapper/lxc-wrapper: cmd/stacker/lxc-wrapper/lxc-wrapper.c
		make -C cmd/stacker/lxc-wrapper LDFLAGS=-static LDLIBS="$(shell pkg-config --static --libs lxc) -lpthread -ldl" lxc-wrapper

feat(sbom): add a directive to generate SBOM for a layer #420

feat(sbom): add a directive to generate SBOM for a layer #420

Conversation

rchincha commented Feb 27, 2023

rchincha commented Aug 14, 2023

smoser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment