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

review: Attest build artifacts #6016

Merged
merged 2 commits into from
Oct 26, 2024
Merged

Conversation

RafDevX
Copy link
Contributor

@RafDevX RafDevX commented Oct 13, 2024

This makes it so all release pipelines attest the artifacts they build, by signing the corresponding checksums. Optionally release workflows may opt out of this behavior, which in this commit is done for nightly as specified by @monperrus on the linked issue.

Closes #5957

@RafDevX
Copy link
Contributor Author

RafDevX commented Oct 13, 2024

We've looked at several different solutions on how we might accomplish what is described in the linked issue. In our opinion, the easiest way to do this would be by simply configuring JReleaser to use cosign, which would be fairly easy and integrate well with the whole process (see docs).

Unfortunately, however, it seems like spoon is already signing releases using JReleaser, counting with this existing configuration:

spoon/jreleaser.yml

Lines 36 to 38 in 25c9f66

signing:
active: ALWAYS
armored: true

It is our understanding that changing signing.mode to COSIGN would, despite solving this particular issue, also turn off GPG signing altogether, which we believe would be detrimental to the release lifecycle (especially when pushing the library to the maven repository) and thus unacceptable. As far as we can tell, there is no (simple) way to get JReleaser to perform both kinds of signing in a single operation.

We thus turned to https://github.com/actions/attest-build-provenance, maintained by GitHub and originally suggested here. This requires adding a new step to the release workflow and trusting yet another dependency, but with version pinning we believe that relying on it is both acceptable and promising.

@ludvigch
Copy link
Contributor

Worth adding is that we have been brainstorming on how to test this properly but haven't come up with any viable ideas. The realease process and this change is inherent to the repository and depends on multiple secrets/assurances. We are unsure of how to test it without creating a fake Maven package and polluting the public sigstore transparency log, which we would prefer not to do.

Therefore we will mark it as ready for review in hopes that you (maintainers) can help come up with ideas on how to confirm that this change works properly and doesn't break anything or if any fixes are required.

@RafDevX RafDevX marked this pull request as ready for review October 13, 2024 16:11
@I-Al-Istannen I-Al-Istannen changed the title Attest build artifacts review: Attest build artifacts Oct 13, 2024
@algomaster99
Copy link
Contributor

@I-Al-Istannen any ideas how we could test this?

@I-Al-Istannen
Copy link
Collaborator

I have absolutely zero clue what anything in this PR does, let alone what the words mean. This is why I requested martin, but I have no idea how active he is here.

I could clone your branch into the spoon repo itself, make it trigger on PR and then have it release a nightly build to the snapshot repository? That way we only pollute a relatively unimportant repository when trying it out, and if it works there it will hopefully also work in spoon proper.

WDYT?

@algomaster99
Copy link
Contributor

I could clone your branch into the spoon repo itself, make it trigger on PR and then have it release a nightly build to the snapshot repository? That way we only pollute a relatively unimportant repository when trying it out, and if it works there it will hopefully also work in spoon proper.

Sounds good for me!

@algomaster99
Copy link
Contributor

algomaster99 commented Oct 19, 2024

#6024 works! We could merge this. @monperrus thoughts? Left some comments here, unrelated to the PR though.

@algomaster99
Copy link
Contributor

algomaster99 commented Oct 22, 2024

@I-Al-Istannen I guess we can merge this. I don't think so we require any changes from authors.

@I-Al-Istannen
Copy link
Collaborator

I am not really sure why we would exclude nightly, but those are only pushed to a snapshot repository anyways.

@algomaster99
Copy link
Contributor

I am not really sure why we would exclude nightly,

@monperrus is there any reason?

@ludvigch
Copy link
Contributor

#6024 works! We could merge this. @monperrus thoughts? Left some comments here, unrelated to the PR though.

We're glad it works! Thanks for the help with testing :)

@monperrus
Copy link
Collaborator

very good progress, thanks! let's do nightly also, it's important for integrity.

RafDevX and others added 2 commits October 26, 2024 16:06
This makes it so all release pipelines attest the artifacts they build,
by signing the corresponding checksums. Optionally release workflows may
opt out of this behavior, which in this commit is done for `nightly` as
specified by @monperrus on the linked issue.

Closes INRIA#5957

Co-authored-by: ludvigch <[email protected]>
Per maintainer feedback, all kinds of release (including nightly) should
attest built artifacts. This commit removes the opt-out functionality
entirely since it is no longer needed (but if necessary in the future
this commit can be reverted at will).
@algomaster99
Copy link
Contributor

@monperrus this is ready for merge after the workflow run passes.

@MartinWitt
Copy link
Collaborator

MartinWitt commented Oct 26, 2024

@I-Al-Istannen

I have absolutely zero clue what anything in this PR does, let alone what the words mean. This is why I requested martin[..]

In short, we provide the ability to verify our artifacts with a signed commit hash. This is stored in a GitHub-powered storage. I never saw this required in any critical software. You can write code for a nuclear power plant without it. So there is a high chance this is written but never read storage. But as long as this action does not crash, we can include it.

@I-Al-Istannen
Copy link
Collaborator

Thanks @MartinWitt. That sounds like enough of an endorsement :D Will merge.

@I-Al-Istannen I-Al-Istannen merged commit 51b5032 into INRIA:master Oct 26, 2024
12 checks passed
@monperrus
Copy link
Collaborator

thanks a lot @RafDevX for the PR and all for converging to merge.

how do one find the link to the rekor attestation for a release?

@algomaster99
Copy link
Contributor

@monperrus I have only seen the URL to it in the build logs - https://github.com/INRIA/spoon/actions/runs/11536652503/job/32113139758#step:5:70.

@ludvigch
Copy link
Contributor

ludvigch commented Nov 3, 2024

@monperrus To add to @algomaster99 the action also puts some links to the attestations stored in the GitHub repository in the workflow run summary - https://github.com/INRIA/spoon/actions/runs/11536652503

Rekor is also searchable with the hash of an attested artifact, for example attestation for spoon-core-11.1.1-beta-11-jar-with-dependencies.jar can be found at -
https://search.sigstore.dev/?hash=804c2ab449cc16052b467edc3ab1f7cf931f8e679685c0e16fab2fcc16ecfb41

And locally one can verify individual files with GitHub cli command:
gh attestation verify spoon-core-11.1.1-beta-11-jar-with-dependencies.jar -R INRIA/spoon

Output:

Loaded digest sha256:804c2ab449cc16052b467edc3ab1f7cf931f8e679685c0e16fab2fcc16ecfb41 for file://spoon-core-11.1.1-beta-11-jar-with-dependencies.jar
Loaded 1 attestation from GitHub API
✓ Verification succeeded!

sha256:804c2ab449cc16052b467edc3ab1f7cf931f8e679685c0e16fab2fcc16ecfb41 was attested by:
REPO         PREDICATE_TYPE                  WORKFLOW                                         
INRIA/spoon  https://slsa.dev/provenance/v1  .github/workflows/jreleaser.yml@refs/heads/master

@monperrus
Copy link
Collaborator

very interesting info @ludvigch

it would be great to document this work, could you create a documentation page supply-chain.md where we document how to set build attestations for Maven and how to use them (your previous message)?

It would be super useful!

@ludvigch
Copy link
Contributor

ludvigch commented Nov 5, 2024

I can document it and open a separate pull request for it towards the end of the week!

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.

push package checskums to sigstore/rekor as part of CD
6 participants