Isolated sign

[LaurentGoderre]

Depends on docker-library/meta-scripts#79

[kipz]

@LaurentGoderre what's the next step on this one?

[LaurentGoderre]

@kipz this is the last part that need to be fixed AFAIK

https://github.com/docker-library/meta/pull/41/files#diff-5c3fa597431eda03ac3339ae6bf7f05e1a50d6fc7333679ec38e21b337cb6721R219-R230

[kipz]

Thanks @LaurentGoderre . What's wrong with it? What's the solution? Are you working on this or should it be me?

[LaurentGoderre]

@kipz I wrote many alternate ways, just trying to get the right one in

[tianon]

I didn't quite get all the way through it, but I've left a bunch of suggestions -- happy to go through them, explain them in more detail, commit them directly, etc as you desire. 🙇 ❤️

(wanted to give you something to work with/through even though I'm out of time for today ❤️)

[tianon]

These should move down to be part of jobs.sign.env or better yet even jobs.sign.steps[].env for the specific step that needs them (given there's only the one).

[LaurentGoderre]

There is only one of the iteration but it would be used more than once once we have the VSA in. It's not as obvious here but I wanted to do it like this to minimize the changrs

[tianon]

A few notes:

• a comment to explain why we're doing all this dance
• slightly different ids
• explicit sha256sum line so set -eo pipefail can fail it correctly
• leaning into the tarball including temp/ just like the build itself does (which will avoid the mkdir later too)
• more defensive upload-artfiact parameters (especially failing the build if it fails to output)

Something I haven't resolved here yet is that this actually won't work if a build specifies Builder: classic, which is totally valid even in the new build system (and will notably affect all Windows image builds right off the bat). 🤔

Suggested change

	- name: Generate Checksum
	id: checksum
	run: |
	cd build
	tar -cvf temp.tar -C temp .
	echo "sha256=$(sha256sum temp.tar)" >> "$GITHUB_OUTPUT"
	- name: Stage artifact
	id: oci
	uses: actions/upload-artifact@v4
	with:
	name: build-oci
	path: |
	build/temp.tar*
	retention-days: 5
	# save the build as an "artifact" so we can sign it in a separate step (and minimize the exposure of the signing credentials); also save a checksum so we can be sure it transmits between the steps accurately
	# TODO what do we do here for "classic" builds like Windows? (no "temp" OCI layout currently)
	- name: Generate Artifact
	id: sha256
	run: |
	tar -cvf temp.tar -C build temp
	sha256="$(sha256sum temp.tar | cut -d' ' -f1)"
	echo "sha256=$sha256" >> "$GITHUB_OUTPUT"
	- name: Upload Artifact
	id: artifact
	uses: actions/upload-artifact@v4
	with:
	name: build-oci
	path: temp.tar
	if-no-files-found: error
	retention-days: 5

[tianon]

If we move the validation / provenance generation to this job (which the more I think about it, the more it makes sense), I'd adjust my suggestion to this:

Suggested change

	- name: Generate Checksum
	id: checksum
	run: |
	cd build
	tar -cvf temp.tar -C temp .
	echo "sha256=$(sha256sum temp.tar)" >> "$GITHUB_OUTPUT"
	- name: Stage artifact
	id: oci
	uses: actions/upload-artifact@v4
	with:
	name: build-oci
	path: |
	build/temp.tar*
	retention-days: 5
	# TODO what do we do here for "classic" builds like Windows? (no "temp" OCI layout currently)
	- name: Validate
	id: validate
	run: |
	scripts="$PWD/.scripts"
	cd build/temp
	indexDigest="$(
	jq -sr -L"$scripts" '
	include "oci";
	# TODO this is going to fail for oci-import images because it validates that .manifests[0].mediaType is an index 🤔 (we need to fix that by adjusting our normalization for those to *also* be an OCI index in an index, which simplifies things here because we get to benefit from more shared assumptions)
	validate_oci_layout
	| .[1] # we do not need the oci-layout anymore (only index.json)
	# "validate_oci_layout" invoked "validate_oci_layout_index" which validated that .manifests has only a single (sane) entry, and that it is an index mediaType (so we only need the digest):
	| .manifests[0]
	| .digest
	' oci-layout index.json
	)"
	[ -n "$indexDigest" ]
	echo "indexDigest=$indexDigest" >> "$GITHUB_OUTPUT"
	shell="$(
	jq -r -L"$scripts" '
	include "oci";
	validate_oci_index
	| .manifests
	| if length | IN(1, 2) then . else
	error("expected only one or two manifests, not \(length)")
	end
	# make sure our objects are in a sane order (arguably, we could instead validate that they are in the correct order, but this is simpler)
	| sort_manifests
	# (now we can assume .manifests[0] is our image and if .manifest[1] exists, it is our attestation)
	| @sh "manifestDigest=\(.manifests[0].digest)",
	@sh "attestationDigest=\(.manifests[1]?.digest // "")"
	' "blobs/${indexDigest//://}"
	)"
	eval "$shell"
	[ -n "$manifestDigest" ] # attestationDigest might be empty (if there was not an attestation manifest)
	echo "manifestDigest=$manifestDigest" | tee -a "$GITHUB_OUTPUT" "$GITHUB_ENV" > /dev/null
	echo "attestationDigest=$attestationDigest" >> "$GITHUB_OUTPUT" # TODO do we even need this?
	- name: Provenance
	env:
	GITHUB_CONTEXT: ${{ toJson(github) }} # NOTE this contains sensitive information, and should not be printed out directly (https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/accessing-contextual-information-about-workflow-runs#github-context)
	RUNNER_CONTEXT: ${{ toJson(runner) }}
	run: |
	jq <<<"$json" -L.scripts '
	include "provenance";
	(env.GITHUB_CONTEXT | fromjson) as $github
	| (env.RUNNER_CONTEXT | fromjson) as $runner
	| github_actions_provenance($github; $runner; env.manifestDigest)
	' | tee build/provenance.json
	# save the build as an "artifact" so we can sign it in a separate step (and minimize the exposure of the signing credentials); also save a checksum so we can be sure it transmits between the steps accurately
	- name: Generate Artifact
	id: sha256
	run: |
	tar -cvf temp.tar -C build temp provenance.json
	sha256="$(sha256sum temp.tar | cut -d' ' -f1)"
	echo "sha256=$sha256" >> "$GITHUB_OUTPUT"
	- name: Upload Artifact
	id: artifact
	uses: actions/upload-artifact@v4
	with:
	name: build-oci
	path: temp.tar
	if-no-files-found: error
	retention-days: 5

(and with this change, we don't actually need to "output" any of the digests, so all those lines could be dropped and only put manifestDigest into GITHUB_ENV for the provenance generation step, but they're also harmless to keep and might be relevant in the future)

[LaurentGoderre]

Joel was adamant that to meet the spec we need to have the provenance generation in a separate job which is why we did it that way

[mrjoelkamp]

👋 It is a bit nuanced and I felt more comfortable generating provenance data in isolation from the build job (to prevent modification). For sure, there is a requirement to isolate access to provenance signing keys from the build job but there is some ambiguity with respect to what process generates the data.

Accuracy: The provenance MUST be generated by the control plane (i.e. within the trust boundary identified in the provenance) and not by a tenant of the build platform (i.e. outside the trust boundary), except as noted below.

The data in the provenance MUST be obtained from the build platform, either because the generator is the build platform or because the provenance generator reads the data directly from the build platform.

source: https://slsa.dev/spec/v1.1/onepage#provenance-generation

[LaurentGoderre]

@mrjoelkamp thank you for these details!!!

[tianon]

Since this is the only place it's referenced, we could move this back here (ie, no env: at all anymore). That has the benefit of putting the pin directly next to the code that uses it (which helps encourage us to verify the usage when updating the pinning):

Suggested change

	"$IMAGE_SIGNER"
	docker/image-signer-verifier:0.6.9@sha256:e38be2b9e6f010cf3432a2772b0e800feee572f7733c6df81e21293cb3e977e0

[LaurentGoderre]

Same as previous one. It will be used more than once when we add VSA.

[tianon]

We don't want this to still run on failure, right? I guess this is how you're managing the issue with the if: I noted above? 😭

[LaurentGoderre]

We want to clear the artifact on failure and there is an extra setting to not clear it for debugging (that's what the of does)

[LaurentGoderre]

Ok, collapsing both comments into this one, I was able to use if: ${{ always() && inputs.pruneArtifact }} which ignores build failures but respect the boolean fort pruning.

[mrjoelkamp]

I replied to a couple of comments, feel free to ping me 🥲