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

Modify the build process to generate a signed SLSA provenance to verify produced artifacts #590

Merged
merged 3 commits into from
Mar 22, 2024

Conversation

jeanchpt
Copy link
Contributor

@jeanchpt jeanchpt commented Mar 7, 2024

Changes introduced with this PR

The work introduced via this pull request focuses on modifying the build workflow main.yml to generate a signed provenance (according to SLSA) for all the artifacts of a release. A special section has been added to the README to explain the verification process.

To explain briefly the changes, a new artifact (multiple.intoto.jsonl) is produced during release time. This document is a signed provenance for every artifact which can be verified according to the README section (Verify provenance).

There is also modifications in both main.yml and nightly.yml to add some comments and also to switch from version tags to hashes when calling external actions.

SLSA is a framework that focuses on the security of the supply chain. It has been mentionned in the following issue. You can refer to this documentation for more information about the framework.

All the following should allow ContainerSSH to be compliant with SLSA build level 3 :

  • Provenance exists and is distributed (through the build platform)
  • Provenance is signed
  • Provenance is unforgeable

Signing keys are handled via SLSA provenance generator through Sigstore. If you want more information feel free to contact us.


By contributing to this repository, I agree to the contribution guidelines.

Copy link
Member

@tsipinakis tsipinakis left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the contribution, couple of quick comments. I still need to check the format of the generated artifacts - it's quite new to me :).

FYI seems like DCO is failing, also no CI on this PR... Weird 🤔

- name: Set up Go
uses: actions/setup-go@v2
uses: actions/setup-go@0c52d547c9bc32b1aa3301fd7a9cb496313a4491 #v5.0.0
Copy link
Member

Choose a reason for hiding this comment

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

What's the reasoning behind replacing all the tags with hashes?

Copy link

Choose a reason for hiding this comment

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

Referencing an action with commit hashes is more secure than referencing with tags.
If a malicious actor is controlling the repo of the action at some point, a tag content could change, a commit content couldn't.

See this blog article on the subject for more details : https://blog.rafaelgss.dev/why-you-should-pin-actions-by-commit-hash

The only exception in the changes we did on the workflows is the SLSA generator which specify in it's documentation that it must be referenced by a tag.

.github/workflows/main.yml Outdated Show resolved Hide resolved
@3C4D
Copy link

3C4D commented Mar 11, 2024

The DCO failed because we forgot to add the suffix Signed-off-by: Author Name <authoremail@example.com> in the commit messages.

The commits are still signed by our GPG keys, as we can see by the annotation "verified" next to them.

Also, the build failed because the repository isn't accepting the slsa action for provenance generation we used in the workflow.
You will probably have to manually accept the dependency in order for the build to succeed (as you can see in the build errors summary https://github.com/ContainerSSH/ContainerSSH/actions/runs/8191458931).

@3C4D
Copy link

3C4D commented Mar 11, 2024

Also, the artifacts produced by the build are almost the same as before. We only modified the .goreleaser file to remove the signature part which is now handled elsewhere.

A test build can be seen on the forked repo : https://github.com/jeanchpt/ContainerSSH/releases/tag/0.1.5

The release contains an additional artifact which is the multiple.intoto.jsonl which contains the provenance.
Also, .sig files aren't anymore in the release since there is no longer a signature done by goreleaser.

@tsipinakis
Copy link
Member

tsipinakis commented Mar 14, 2024

A test build can be seen on the forked repo : https://github.com/jeanchpt/ContainerSSH/releases/tag/0.1.5

Thanks, this plus some reading up on the topic was quite enlightening on the uses and advantages of slsa. To my understanding the main advantage is that the trust chain now extends all the way to the builder and is dependent on the build service to sign the artifacts. This is great and for sure extends the verifiability of the artifacts however it also shifts the authority from the maintainers holding the primary keys to the service itself. Which brings me to...

Also, .sig files aren't anymore in the release since there is no longer a signature done by goreleaser.

As per the above, I think it's a good idea to keep the gpg signatures as well on top of SLSA.

  1. As mentioned above slsa signatures are controlled by the service, not the maintainers or the person triggering the build. With GPG the maintainers of the project have full control of the signing keys and can easily revoke further access or move it to a new service and in this scenario it provides a verifiable chain to users i.e. it's the same project as it is signed by the same key/person. This is not possible with SLSA.
  2. GPG signatures are easily verifiable using only the default minimal installation of any linux distribution, SLSA isn't :)

So lets bring back GPG as well I'd say. Maybe to try to decrease the number of artifacts published a SHA512SUMs and SHA512SUMS.sig can be used. To be seen in the future.

The DCO failed because we forgot to add the suffix Signed-off-by: Author Name authoremail@example.com in the commit messages.

Please fix that :) Signing off is a requirement to ensure that you accept the contributing guidelines.

Signed-off-by: Jean Chaput <jean.chaput@protonmail.com>
Signed-off-by: Jean Chaput <jean.chaput@protonmail.com>
@3C4D
Copy link

3C4D commented Mar 15, 2024

Hi, we've done some modifications according to your remarks :

  • We changed the commits to correspond to the DCO requirements.
  • We brought back the GPG signature (in the goreleaser file) on the side of SLSA provenance generation and signature in order for the verification process to be more user friendly.
  • As well as you talked about a single sums file to be signed, we saw that goreleaser is already generating a file that contains the checksums of all artifacts. We are only signing this file as signing the others would be redundant.

The generated checksum file is called containerssh_[VERSION]_checksums.txt, so an only .sig file is generated from the build.

@tsipinakis
Copy link
Member

Whoops messed up a force push to try to make CI run, and sadly don't have the latest commit in my local repo. Can you git push --force from your local copy to bring back the latest commit?

@jeanchpt
Copy link
Contributor Author

Hello, that's done, my latest commit is restored :)

actions: read # To read the workflow path.
id-token: write # To sign the provenance.
contents: write # To add assets to a release.
uses: slsa-framework/slsa-github-generator/.github/workflows/generator_generic_slsa3.yml@v1.9.0
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
uses: slsa-framework/slsa-github-generator/.github/workflows/generator_generic_slsa3.yml@v1.9.0
uses: slsa-framework/slsa-github-generator/.github/workflows/generator_generic_slsa3.yml@v1.10.0

The reason it took so long to review this is I was trying to find out why my simple test was failing, turns out 1.9.0 seems to be broken due to this

Apply this and all good to merge!

Copy link

Choose a reason for hiding this comment

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

Indeed we saw it too, our past tests were beginning to fail but SLSA released the version 12 hours ago.

Thank you for noticing it, I changed the version.

.goreleaser.yaml Outdated
github:
owner: containerssh
name: containerssh
#github:
Copy link
Member

Choose a reason for hiding this comment

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

Ah, just noticed this slipped in as well, you can drop the commit.

Signed-off-by: Jean Chaput <jean.chaput@protonmail.com>
@jeanchpt
Copy link
Contributor Author

I reset the commit to remove the commenting error that was introduced with it. Now everything should be fine :)

Copy link
Member

@tsipinakis tsipinakis left a comment

Choose a reason for hiding this comment

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

LGTM :)

@tsipinakis tsipinakis merged commit d73a314 into ContainerSSH:main Mar 22, 2024
1 check passed
@jeanchpt jeanchpt deleted the slsa branch March 22, 2024 10:38
@jeanchpt jeanchpt restored the slsa branch March 22, 2024 10:38
@jeanchpt jeanchpt deleted the slsa branch March 22, 2024 10:38
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.

3 participants