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

Specify the builder id for provenance #61

Merged
merged 1 commit into from
Oct 11, 2024

Conversation

LaurentGoderre
Copy link
Member

@LaurentGoderre LaurentGoderre commented Jun 27, 2024

When generating a provenance using the GitHub Action Build-Push Action the action sets the builder id as the URL for the current build. This replicates this behavior.

Example
https://oci.dag.dev/?blob=docker/image-signer-verifier@sha256:56e95d5cbc3afae22c0023f0e386cd04254de1ecd6b98b0b1da483b2a28b1c73&mt=application%2Fvnd.in-toto%2Bjson&size=11949

"builder": {
  "id": "https://github.com/docker/image-signer-verifier/actions/runs/9620065389/attempts/1"
},

We are wondering however if the GitHub Action run url is the best value to use of if a different URL would be better suited.

Copy link

@jonnystoten jonnystoten left a comment

Choose a reason for hiding this comment

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

Thanks @LaurentGoderre ! This is helpful for us when verifying the provenance of DOI because this is a common value to look for across all DOI with provenance attestations.

To fully replicate the docker/build-push-action behavior we'd need to include the "attempt" in the URL too, see their implementation here.

meta.jq Outdated Show resolved Hide resolved
@tianon
Copy link
Member

tianon commented Jul 1, 2024

This is helpful for us when verifying the provenance of DOI because this is a common value to look for across all DOI with provenance attestations.

Can you please elaborate more on this? In what way will it be used/validated? Just that it starts with https://github.com/docker-library/meta/actions/...?

They won't necessarily all look that way -- this PR would also need to account for builds that happen on Jenkins, which does have similar environment variables, but as they won't be terribly stable / long-lived URLs in either case, I'd like to understand more about what the actual underlying use case is before I can reasonably opine on what the best solution/value is.

@tianon
Copy link
Member

tianon commented Oct 9, 2024

We can close this in favor of #76 now, right? 👀

@LaurentGoderre
Copy link
Member Author

@mrjoelkamp ?

@mrjoelkamp
Copy link
Contributor

We can close this in favor of #76 now, right?

We could close this, but there is still some value in keeping it if we are continuing to generate buildkit provenance attestations so that the value is populated with something useful instead of being empty.

It seems reasonable to me to keep this change so long as we are continuing to generate buildkit provenance.

@LaurentGoderre
Copy link
Member Author

Ah! @tianon the gha provenance has the right value but the buildkit doesn't. This one populates the buildkit one.

@tianon
Copy link
Member

tianon commented Oct 10, 2024

Right, that makes sense, but doesn't help me with this: 😅

They won't necessarily all look that way -- this PR would also need to account for builds that happen on Jenkins, which does have similar environment variables, but as they won't be terribly stable / long-lived URLs in either case, I'd like to understand more about what the actual underlying use case is before I can reasonably opine on what the best solution/value is.

I don't think the URL to the action is a good default (especially now that I have a better understanding of the purpose of builder.id thanks to our discussions around/in #76), and we'll need to either not set this at all on Jenkins or choose a suitable value there too (in this same file/line), which is all together why I'm thinking we might be better off just not setting a builder.id in our BuildKit provenance since the provenance we are going to generate (via #76) has a suitable value that we've put a lot of thought and review into more wholistically than we can do with the BuildKit provenance. 🙇

@mrjoelkamp
Copy link
Contributor

I must have missed that comment. Because this file is shared with Jenkins, it does make more sense now to leave it empty or hardcode it to some string value (e.g. buildkit via $builder) but we wouldn't really be using it for anything that I know of.

Having not spent much time reviewing version v0.2 of the SLSA spec, I did check that it requires the builder.id so it does seem odd/wrong to leave it empty 🤔

Without over thinking or complicating it, setting builder.id = "buildkit" would suffice, thoughts? It's not much better than leaving it "" but at least it populates a required field 🤷 .

@tianon
Copy link
Member

tianon commented Oct 10, 2024

Hmm, I see. I don't think we care too much about this document being "compliant", especially to an officially superseded spec, but setting it to something and not putting too much stock in the particular value is probably fine, yeah. Since it's supposed to be a URI, maybe https://github.com/docker-library or similar? (so it's still vague, but compliant and accurate to "the entity that executed the invocation")

We should probably follow up with the Docker Docs team to have something about --provenance=...,builder-id=... added to https://docs.docker.com/build/metadata/attestations/slsa-provenance/ too 😬
(https://docs.docker.com/build/metadata/attestations/slsa-definitions/ talks about the value and what it means, but not how to set it 🙈)

So, if we do that instead of a generic "buildkit" (which isn't a URI 😭), then we need to shift it over to doi.jq (because meta.jq is, and is intended to be, generic):

# https://github.com/docker-library/meta-scripts/pull/61 (for lack of better documentation for setting this in buildkit)
# https://slsa.dev/provenance/v0.2#builder.id
def buildkit_provenance_builder_id:
	"https://github.com/docker-library"
;

and then something like this in meta.jq:

	@sh "--provenance=\([ "mode=max", "builder-id=\(buildkit_provenance_builder_id)" ] | @csv)",

The final raw output of which will be something like --provenance='"mode=max","builder-id=https://github.com/docker-library"' -- appropriate shell quoting and CSV quoting, although it would be totally reasonable (and generate slightly nicer looking final shell syntax) to pull mode=max out of the CSV quoting, something like this:

	@sh "--provenance=mode=max,\([ "builder-id=\(buildkit_provenance_builder_id)" ] | @csv)",

or even go further and just assume our builder-id shouldn't ever contain quotes or a comma, and go all the way to this (with only shell quoting, no CSV quoting):

	@sh "--provenance=mode=max,builder-id=\(buildkit_provenance_builder_id)",

The end result of which would be --provenance=mode=max,builder-id='https://github.com/docker-library' (only shell quoting).

@tianon
Copy link
Member

tianon commented Oct 10, 2024

Lol, I just had to hit "comment" to notice https://docs.docker.com/build/metadata/attestations/slsa-definitions/#:~:text=This%20value%20can%20be%20set%20using,attestation%20parameter. (but it's very much a footnote 😅)

This value can be set using the builder-id attestation parameter.

@mrjoelkamp
Copy link
Contributor

Since it's supposed to be a URI, maybe https://github.com/docker-library or similar?

👍 That would be even better! Sounds good to me, and feels more complete than the empty string.

@LaurentGoderre LaurentGoderre marked this pull request as ready for review October 11, 2024 14:22
@tianon tianon merged commit e22b5e2 into docker-library:main Oct 11, 2024
1 check passed
@LaurentGoderre LaurentGoderre deleted the provenance-builder branch October 11, 2024 18:36
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.

4 participants