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

Send list of extra SBOM scanner to use #17699

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

LaurentGoderre
Copy link
Member

No description provided.


local scanners
scanners="php-composer-lock,erlang-otp-application,lua-rock-cataloger,swipl-pack-cataloger,opam-cataloger"
vars="$(_jq_setenv <<<"$vars" BASHBREW_BUILDKIT_EXTRA_SCANNERS "$scanners")"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The EXTRA_SCANNERS configuration knob is something that's specific to docker/scout-sbom-indexer, right? (ie, it wouldn't be likely to be supported by any other scanner?)

So, relevant to docker-library/meta-scripts#86 (the implementation of this on the other side), I think this might be cleaner to use/abuse the fact that BASHBREW_BUILDKIT_SBOM_GENERATOR gets embedded directly as-is, since EXTRA_SCANNERS is technically a parameter of that generator:

			local sbomGenerator
			# https://hub.docker.com/r/docker/scout-sbom-indexer/tags
			sbomGenerator="$(grep <<<"$externalPins" -m1 '^docker/scout-sbom-indexer:')"
			sbomGenerator="$(_resolve_external_pins "$sbomGenerator")"
			# https://github.com/moby/buildkit/pull/5372 - "EXTRA_SCANNERS" is an optional parameter to the Scout SBOM Indexer
			sbomGenerator+=',"EXTRA_SCANNERS=php-composer-lock,erlang-otp-application,lua-rock-cataloger,swipl-pack-cataloger,opam-cataloger"'
			vars="$(_jq_setenv <<<"$vars" BASHBREW_BUILDKIT_SBOM_GENERATOR "$sbomGenerator")"

(A side benefit of doing it here is that it's a lot easier to get the CSV-vs-shell-quoting right so those , don't trip us up 😅)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we do that, we need to include the generator= part here as well because if the CSV parsing . The syntax is "generator=foo","arg=bar"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now, we're only shell quoting the generator, not CSV-quoting it, and in CSV quoting, we can optionally quote some values while not quoting others.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can see that more accurately in https://github.com/docker-library/meta-scripts/blob/2084a63dca2ed7a0e0e0f7992a205333d32bfb56/.test/meta-commands/out.sh#L147 -- we're just doing --sbom=generator="$BASHBREW_BUILDKIT_SBOM_GENERATOR" which will expand to --sbom=generator=whatever-the-env-value-is as a single argument to docker buildx build, so any ,"foo" at the end of it will be passed as-is to the --sbom flag.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was going to show this working, but I'm not actually sure the backport is working? Does it also need updates to buildx? 😬

$ docker buildx create --name foo --driver-opt image=tianon/buildkit --bootstrap
...
$ docker buildx build --builder foo --sbom=generator='docker/scout-sbom-indexer:1' --output type=oci,dest=/dev/null --no-cache - <<<'FROM swipl' |& grep 'INFO Indexed '
INFO Indexed 217 packages
$ docker buildx build --builder foo --sbom=generator='docker/scout-sbom-indexer:1,"EXTRA_SCANNERS=php-composer-lock,erlang-otp-application,lua-rock-cataloger,swipl-pack-cataloger,opam-cataloger"' --output type=oci,dest=/dev/null --no-cache - <<<'FROM swipl' |& grep 'INFO Indexed '
INFO Indexed 217 packages

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, false alarm, that's actually because docker/scout-sbom-indexer:1 doesn't actually support an EXTRA_SCANNERS variable yet; testing with your image works successfully and demonstrates what I mean:

$ docker buildx build --builder foo --sbom=generator='laurentgoderre689/scout-sbom-indexer:extra@sha256:17d0cba7e0a840e84c8e565ca0163ccac8ae7794775ca92a2f8fda71e227f08a' --output type=oci,dest=/dev/null --no-cache - <<<'FROM swipl' |& grep 'INFO Indexed '
INFO Indexed 217 packages
$ docker buildx build --builder foo --sbom=generator='laurentgoderre689/scout-sbom-indexer:extra@sha256:17d0cba7e0a840e84c8e565ca0163ccac8ae7794775ca92a2f8fda71e227f08a,"EXTRA_SCANNERS=php-composer-lock,erlang-otp-application,lua-rock-cataloger,swipl-pack-cataloger,opam-cataloger"' --output type=oci,dest=/dev/null --no-cache - <<<'FROM swipl' |& grep 'INFO Indexed '
INFO Indexed 221 packages

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To further illustrate, docker-library/meta-scripts#86 won't work as-is; this line:

"--sbom=\"generator=$BASHBREW_BUILDKIT_SBOM_GENERATOR\",\"EXTRA_SCANNERS=$BASHBREW_BUILDKIT_EXTRA_SCANNERS"

would need to instead be something like:

"--sbom=\"generator=$BASHBREW_BUILDKIT_SBOM_GENERATOR,\\\"EXTRA_SCANNERS=$BASHBREW_BUILDKIT_EXTRA_SCANNERS\\\"\""

(which is what I mean by it being much easier to get the quoting correct if we just do it here instead, where we've specified the SBOM generator and thus know that our EXTRA_SCANNERS parameter actually applies)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I tried it, mixing quoting some fields and not other didn't seem to work but maybe I did it wrong

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's definitely tricky to get right because of the interaction of the shell doing quoting (and needing to, for variable expansion to work correctly) and the quoting required for the CSV-style microformat used by the command we're passing this value to. 😬

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's another way to illustrate this:

$ # just shell quoting
$ sh -c 'echo "$@"' -- --sbom="generator=foo","EXTRA_SCANNERS=bar"
--sbom=generator=foo,EXTRA_SCANNERS=bar

$ # just shell quoting generator, but *also* CSV-quoting EXTRA_SCANNERS
$ sh -c 'echo "$@"' -- --sbom="generator=foo","\"EXTRA_SCANNERS=bar\""
--sbom=generator=foo,"EXTRA_SCANNERS=bar"

@LaurentGoderre LaurentGoderre marked this pull request as ready for review October 10, 2024 16:32
@LaurentGoderre LaurentGoderre requested a review from a team as a code owner October 10, 2024 16:32
Comment on lines 67 to 71
# https://hub.docker.com/r/docker/scout-sbom-indexer/tags
sbomTag="$(grep <<<"$externalPins" -m1 '^docker/scout-sbom-indexer:')"
sbomTag="$(_resolve_external_pins "$sbomTag")"
vars="$(_jq_setenv <<<"$vars" BASHBREW_BUILDKIT_SBOM_GENERATOR "$sbomTag")"
scanners="php-composer-lock,erlang-otp-application,lua-rock-cataloger,swipl-pack-cataloger,opam-cataloger"
vars="$(_jq_setenv <<<"$vars" BASHBREW_BUILDKIT_SBOM_GENERATOR "$sbomTag,\"EXTRA_SCANNERS=$scanners\"")"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Repeating a few minor suggestions, this time with more explanation for each one (so they're hopefully easier to understand/discuss): 😇 ❤️

  • renaming sbomTag to sbomGenerator, since that's the variable it's setting so it's a more appropriate name

  • using Bash's += to append the scanners directly to that variable to avoid a second variable and so we can use single-quotes so that the CSV quoting is easier to get right (no escaping necessary)

  • adding a comment pointing to Added way to configure SBOM scanner moby/buildkit#5372 so we can easily find where this functionality was added to BuildKit + a note about what "EXTRA_SCANNERS" is (because it's not documented publicly anywhere)

			local sbomGenerator
			# https://hub.docker.com/r/docker/scout-sbom-indexer/tags
			sbomGenerator="$(grep <<<"$externalPins" -m1 '^docker/scout-sbom-indexer:')"
			sbomGenerator="$(_resolve_external_pins "$sbomGenerator")"
			# https://github.com/moby/buildkit/pull/5372 - "EXTRA_SCANNERS" is an optional parameter to the Scout SBOM Indexer
			sbomGenerator+=',"EXTRA_SCANNERS=php-composer-lock,erlang-otp-application,lua-rock-cataloger,swipl-pack-cataloger,opam-cataloger"'
			vars="$(_jq_setenv <<<"$vars" BASHBREW_BUILDKIT_SBOM_GENERATOR "$sbomGenerator")"

I also think we should probably wait to merge this until we have a new release of docker/scout-sbom-indexer that includes support for EXTRA_SCANNERS so we don't forget that this isn't actually doing anything yet and scratch our heads later. 🤔 Perhaps even include the swap from the 1-doi tag to the 1 tag?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants