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

Build vendor artifact #11764

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

Build vendor artifact #11764

wants to merge 1 commit into from

Conversation

catap
Copy link

@catap catap commented Apr 24, 2024

Closes: #11760

@catap
Copy link
Author

catap commented Apr 24, 2024

@ndeloof here it is

Copy link
Contributor

@ndeloof ndeloof left a comment

Choose a reason for hiding this comment

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

LGTM but should be declared in the merge workflow so this run for commits on main branch and tags

-
name: Build vendor
run: |
go mod tidy
Copy link
Contributor

Choose a reason for hiding this comment

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

tidy is not relevant here, as we enforce go.sum to be up-to-date as part of the PR workflow

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

@catap
Copy link
Author

catap commented Apr 24, 2024

LGTM but should be declared in the merge workflow so this run for commits on main branch and tags

ci workflow is triggered for by push into main brnach and tags as well. Also, this workflow makes all artifacts and it seems for me logical that it should be used to produce this one.

Have I missed something?

@ndeloof
Copy link
Contributor

ndeloof commented Apr 24, 2024

ci workflow is triggered by push to main but also by pull_requests, and this doesn't make sense to build this src archive for PRs
merge workflow is used for stuff to only happen on main branch and releases, like building bin image

@thaJeztah
Copy link
Member

We should probably check if there's additional verification needed for these packages. The standard source tarballs on GitHub releases are produced from git, and therefore should always match the source code. This PR constructs a custom tar; while it's produced through GitHub actions, it's effectively still a "manual" upload, so we should check if there's ways to verify provenance and integrity.

Recent events with the xz backdoor showed that manually / custom uploaded sources were at the root of that backdoor (the xz project instructed users to not use the default tarballs produced by GitHub, but the ones that were manually uploaded).

cc @rachel-taylor-docker @gabriellavengeo

@catap
Copy link
Author

catap commented Apr 24, 2024

@thaJeztah this is one of root cause why I'm doing this.

Right now OpenBSD port for docker-compose depends on .tar.gz which I build and host on my machine. Everyone should trust that I haven't been compromised and doesn't inject something very bad into this .tar.gz and has we seen such injection can be quite complicated.

So, the right way to do it is download from the same source where it sources is produced becuase this should be trusted source by defenition.

Regarding the way: as soon as I know go mod vendor respects checksums inside go.mod that should prevent from packing bad things. Or, let me say it that way: it should pack the same things which user should get if he run it locally.

@catap
Copy link
Author

catap commented Apr 24, 2024

ci workflow is triggered by push to main but also by pull_requests, and this doesn't make sense to build this src archive for PRs merge workflow is used for stuff to only happen on main branch and releases, like building bin image

To keep things simpler I've added if inside this step.

Otherwise I need to move and duplicate code inside merge workflow which seems wrong from my point of view.

Closes: docker#11760

Signed-off-by: Kirill A. Korinsky <kirill@korins.ky>
@thaJeztah
Copy link
Member

Right now OpenBSD port for docker-compose depends on .tar.gz which I build and host on my machine. Everyone should trust that I haven't been compromised and doesn't inject something very bad into this .tar.gz and has we seen such injection can be quite complicated.

Yes, anyone producing binaries / artifacts for others to consume must have provenance. I think that goes beyond just these tars, and there would be many other ways for that build-environment / infra to be comprimised.

Regarding the way: as soon as I know go mod vendor respects checksums inside go.mod that should prevent from packing bad things. Or, let me say it that way: it should pack the same things which user should get if he run it locally.

Correct; it won't protect against a compromised tarball that has go.mod and go.sum tampered with though; so I'm looking if there's any way to have provenance for the uploaded tarball to verify if it's produced by the GitHub action, and not replaced in other ways.

The default tarballs produced by GitHub always match the git-commit of the release, but for other artifacts that's not the case, so some validation may be necessary for that.

@catap
Copy link
Author

catap commented Apr 24, 2024

Regarding the way: as soon as I know go mod vendor respects checksums inside go.mod that should prevent from packing bad things. Or, let me say it that way: it should pack the same things which user should get if he run it locally.

Correct; it won't protect against a compromised tarball that has go.mod and go.sum tampered with though; so I'm looking if there's any way to have provenance for the uploaded tarball to verify if it's produced by the GitHub action, and not replaced in other ways.

The default tarballs produced by GitHub always match the git-commit of the release, but for other artifacts that's not the case, so some validation may be necessary for that.

I haven't see any differences with producing .tar.gz by this job with producing binary artifacts by the same github actions.

Second point: GitHub and other git hosting sometimes changes produced tar.gz from tag and they officialy confirm it, see: https://github.blog/2023-02-21-update-on-the-future-stability-of-source-code-archives-and-hashes/ and to avoid such mess they suggested to attach artifacts to releases.

@catap
Copy link
Author

catap commented Apr 30, 2024

just a ping

tar czf bin/release/docker-compose-vendor.tar.gz vendor
-
name: Upload artifacts
uses: actions/upload-artifact@v3

Choose a reason for hiding this comment

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

Is there a reason that we use actions/upload-artifact@v3 instead of v4? According to the action README, v3 will be deprecated in November.

Choose a reason for hiding this comment

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

Speaking of v4, some of the breaking changed in it relate to my other comment bellow about the aritfact name.

2. Uploading to the same named Artifact multiple times.

Due to how Artifacts are created in this new version, it is no longer possible to upload to the same named Artifact multiple times. You must either split the uploads into multiple Artifacts with different names, or only upload once. Otherwise you will encounter an error.

Copy link
Contributor

@ndeloof ndeloof May 6, 2024

Choose a reason for hiding this comment

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

aritfact name must be unique for a built commit, I don't think this has any impact for this workflow.

We still use upload-artifact@v3 on this repo as this breaking change require the whole workflow to be revisited to support the new behavior (see https://github.com/docker/compose/pull/11662/files)

Choose a reason for hiding this comment

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

aritfact name must be unique for a built commit, I don't think this has any impact for this workflow.

Ah ok, then this should be fine.

@ndeloof
Copy link
Contributor

ndeloof commented May 6, 2024

Can you look into https://docs.github.com/en/actions/security-guides/using-artifact-attestations-to-establish-provenance-for-builds#generating-build-provenance-for-binaries so this additional artifact gets provenance attestation and you can safely rely on this for an offline build ?

@catap
Copy link
Author

catap commented May 6, 2024 via email

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.

Release with all vendor dependencies
4 participants