Skip to content

Conversation

@AkihiroSuda
Copy link
Member

@AkihiroSuda AkihiroSuda commented Mar 20, 2025

- What I did

Verify the tags of containerd*, runc, tini, and rootlesskit with their commit hashes for tolerance to compromise of the tags.

These binaries are depeneded by docker-ce-packaging here: https://github.com/docker/docker-ce-packaging/blob/7e726fa319c261676d06b6ae10c04a3df80e4c48/static/Makefile#L43-L58

- How I did it

Used https://github.com/containerd/nerdctl/blob/v2.0.4/hack/git-checkout-tag-with-hash.sh

- How to verify it

Build Dockerfile and Dockerfile.simple

- Human readable description for the release notes

- A picture of a cute animal (not mandatory but encouraged)

@AkihiroSuda AkihiroSuda requested a review from tianon as a code owner March 20, 2025 21:29
@AkihiroSuda AkihiroSuda force-pushed the git-checkout-tag-with-hash branch 2 times, most recently from eb35fa3 to cf544a7 Compare March 20, 2025 21:32
@tianon
Copy link
Member

tianon commented Mar 20, 2025

Any idea whether Git upstream has considered something like this in Git itself? It honestly seems a little janky for us to solve this problem with a wrapper script when it's a fundamental issue across the whole ecosystem of Git consumers.

(Also, all these "install" scripts are currently mostly usable outside the "canonical" build environment, and I would definitely prefer they stay that way, so relying on some script added in the Dockerfile is DOA IMO, but perhaps other maintainers feel differently.)

@AkihiroSuda AkihiroSuda force-pushed the git-checkout-tag-with-hash branch 2 times, most recently from 852e8d9 to 812b89b Compare March 20, 2025 23:45
@AkihiroSuda
Copy link
Member Author

Any idea whether Git upstream has considered something like this in Git itself?

idk, but I'm sure I'm not the first one who wanted "git checkout TAG@HASH".
Maybe somebody has already proposed and got rejected in the past?
Not sure how to search it in https://lore.kernel.org/git/ though.

It honestly seems a little janky for us to solve this problem with a wrapper script when it's a fundamental issue across the whole ecosystem of Git consumers.

I guess we should send a proposal to the Git upstream if nobody has really tried it, however, it will take a years until the feature gets deployed to all Git consumers.
So anyway we need a quick workaround that can be adopted right now.

(Also, all these "install" scripts are currently mostly usable outside the "canonical" build environment, and I would definitely prefer they stay that way, so relying on some script added in the Dockerfile is DOA IMO, but perhaps other maintainers feel differently.)

Replaced a wrapper invocation with a shell function.

@AkihiroSuda
Copy link
Member Author

@tonistiigi
Can we merge this as a workaround until merging moby/buildkit#5871 in a buildx release?

@AkihiroSuda AkihiroSuda force-pushed the git-checkout-tag-with-hash branch from 74c8b51 to d0916fa Compare March 27, 2025 09:45
@AkihiroSuda
Copy link
Member Author

@tonistiigi @thaJeztah PTAL

Copy link
Member

@tonistiigi tonistiigi left a comment

Choose a reason for hiding this comment

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

Not blocking this, but can you help with moby/buildkit#5871 ?

@AkihiroSuda
Copy link
Member Author

Not blocking this, but can you help with moby/buildkit#5871 ?

Sure, after implementing its dependency moby/buildkit#4905 (PR moby/buildkit#5903)

Verify the tags of containerd*, runc, tini, and rootlesskit with
their commit hashes for tolerance to compromise of the tags.

These binaries are depeneded by docker-ce-packaging here:
https://github.com/docker/docker-ce-packaging/blob/7e726fa319c261676d06b6ae10c04a3df80e4c48/static/Makefile#L43-L58

Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
@AkihiroSuda AkihiroSuda force-pushed the git-checkout-tag-with-hash branch from d0916fa to 2755796 Compare April 16, 2025 05:30
@AkihiroSuda
Copy link
Member Author

Rebased

# are built from a commit from the master branch.
ARG CONTAINERD_VERSION=v1.7.27
RUN git fetch -q --depth 1 origin "${CONTAINERD_VERSION}" +refs/tags/*:refs/tags/* && git checkout -q FETCH_HEAD
ARG CONTAINERD_VERSION=v1.7.27@05044ec0a9a75232cad458027ca83437aae3f4da
Copy link
Member

Choose a reason for hiding this comment

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

Wondering if, for now, we should keep these as separate args, e.g.;

Suggested change
ARG CONTAINERD_VERSION=v1.7.27@05044ec0a9a75232cad458027ca83437aae3f4da
ARG CONTAINERD_VERSION=v1.7.27
ARG CONTAINERD_COMMIT=05044ec0a9a75232cad458027ca83437aae3f4da

Or (including the algorithm);

Suggested change
ARG CONTAINERD_VERSION=v1.7.27@05044ec0a9a75232cad458027ca83437aae3f4da
ARG CONTAINERD_VERSION=v1.7.27
ARG CONTAINERD_COMMIT=sha256:05044ec0a9a75232cad458027ca83437aae3f4da

(could be CONTAINERD_HASH or CONTAINERD_GIT_HASH if we think that's a better name)

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't prefer to inflate the number of the args.
Also, splitting the arg will break existing docker build --build-arg CONTAINERD_VERSION=... scripts

@thaJeztah
Copy link
Member

Would indeed be great if something like this would be accepted by Git upstream as a format.

Even with that though, I'm wondering if this approach would technically still be open to TOCTOU-like scenarios, i.e., as a contributor I would have to update the tag and hash, but if a tag would've been compromised by the time I open the PR, I would be codifying the bad hash (how would I be able to detect that I picked the "bad" hash?).

I guess in an ideal world, parties like GitHub and GitLab (but could be someone else) would come with a service similar to Go's sumdb; i.e., tags would not just be immutable through branch-protection rules (or otherwise), but also through a secondary system. When a tag is pushed to a repository, their "sumdb" would register the hash; that registered has would be immutable (so that a forced update to a tag would not update the hash), and when verifying, I could verify if my tag's hash would match the one in that "sumdb" and print a warning / error that the tag.

This is what go modules does when a module version's digest doesn't match what was registered earlier;

GOPRIVATE=github.com/docker/ go mod tidy
verifying github.com/<SOME MODULE>: checksum mismatch
	downloaded: h1:aM6XjGnRAH423IRdlJfG4t8iARHTagqxw4uvcq7rH9Q=
	go.sum:     h1:XhibuNKzaAJo5/2AY/ZXk4Fce21fQyzD7/uTn2WgFsQ=

SECURITY ERROR
This download does NOT match an earlier download recorded in go.sum.
The bits may have been replaced on the origin server, or an attacker may
have intercepted the download attempt.

For more information, see 'go help module-auth'.

in the example above, it was a private module, so the only check was against my local go.sum.

Would that be something that could be done through the "source policy" feature provided by BuildKit? I know you proposed that at the time, and there was something merged, but I never used it;

@AkihiroSuda
Copy link
Member Author

Even with that though, I'm wondering if this approach would technically still be open to TOCTOU-like scenarios, i.e., as a contributor I would have to update the tag and hash, but if a tag would've been compromised by the time I open the PR, I would be codifying the bad hash (how would I be able to detect that I picked the "bad" hash?).

Right. The hash still has to be manually reviewed.

source policy

This could work, but harder to maintain

@AkihiroSuda
Copy link
Member Author

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