Built docker image/manifest digest is not exposed to image signing step, leading to potential race condition vulnerability

[ethan-lowman-dd]

Is your feature request related to a problem? Please describe.

Currently, the Docker image digest built by dockers: ... is not exposed to docker_signs: ... in the environment variables. If the image tag in the registry is modified after pushing and before signing, signer tools that re-pull an image to sign it (including cosign) will potentially sign an image different from the one that was produced by goreleaser.

Describe the solution you'd like

Docker exposes the --metadata-file flag to docker build yields a file that looks like this in the goreleaserdocker temp dir:

{
  "containerimage.digest": "sha256:ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff"
}

If we expose this built digest to the docker_signs tools via an environment variable artifactDigest, users would have the ability to pass the digest to the signing tool and avoid any chance of a race condition. In my opinion, this should be the default behavior.

docker_signs:
  - artifacts: all
    cmd: cosign
    args: ["sign", "${artifact}@${artifactDigest}"]

Describe alternatives you've considered

Assuming you're using a tool like cosign that takes an image as an argument, you can fetch the digest from the local Docker daemon and pass that to the signer tool like so:

docker_signs:
  - artifacts: all
    cmd: sh
    args:
      - -c
      - cosign sign ${artifact}@$(docker inspect ${artifact} -f '{{"{{ (index .RepoDigests 0) }}"}}' | cut -d"@" -f2)

I'm not 100% sure this is actually correct though -- docker inspect returns a list of digests for a given tag, which suggests there may be conditions in which multiple digests are returned for a single tag in the Docker daemon's image store. I couldn't find any such case in a quick code search of Docker, however.

This is quite a hack and it should be easier to avoid the race condition.

Search

• I did search for other open and closed issues before opening this.

Code of Conduct

• I agree to follow this project's Code of Conduct

Additional context

• If cases where users are already passing the --metadata-file to the docker build command via build_flag_templates, it may not be possible to reliably extract the digest from the docker build.
• Discourage signing references to images that aren't digests sigstore/cosign#2047

[caarlos0]

interesting, I had the impression cosign wouldn't pull the image if its already present, though... do you have a doc/code link confirming that?

if thats what happens, I'm all in for looking into this.

[ethan-lowman-dd]

This issue describes the race condition: sigstore/cosign#2047

[ethan-lowman-dd]

One downside of the --metadata-file approach is that it's not supported to the docker manifest builder command. One alternate solution that would work for both images and digests, without the issue of multiple --metadata-file flags, is to do the Go equivalent of docker inspect ${artifact} using Client.ImageInspectWithRaw right before shelling out to the signing command. If the local Docker daemon were untrustworthy, then the image that's pushed and signed would be untrustworthy, so I think using the local Docker daemon is a safe way to resolve the digest.

[ethan-lowman-dd]

Unfortunately Client.ImageInspectWithRaw doesn't work for manifest lists and there's no exported Docker API for getting digests for manifest digests.

[caarlos0]

I think we can literally run docker inspect after building or before signing the image/manifest and parse the output, can't we?

[ethan-lowman-dd]

From my tests, docker inspect doesn't return results for manifests built with docker manifest create, since a pushed manifest isn't in the image store, it's just a local directory on disk in ~/.docker/manifests (see here). You could do docker manifest inspect which returns the raw manifest, but you have to strip the trailing newline and calculate the digest yourself. docker manifest inspect does favor the local manifest list, but falls back on a remote manifest list if the local one is missing. This usually shouldn't happen unless you pass the --purge flag to docker manifest push via the goreleaser push_flags config. That might be an okay compromise, if warnings are logged. Alternatively, it might be better to just contribute something to the Docker CLI that exposes the local manifest digest in a reliable way. I'm not sure how likely that is to be merged, but maybe worth a try.

[caarlos0]

hmm, understood, so right now, nothing we can do here, right?

maybe worth adding a note to our docs about it?

[gal-legit]

Actually, the digest should be fetched from the output of docker push.
I demonstrate it in this blog post, which I wrote as a follow-up to my report in SigStore's Slack channel that led to sigstore/cosign#2047.
I checked goreleaser's code briefly, so I hope I got things right:
It seems that the fix takes changing the following function to also fetch stdout and parse it:

func (i dockerImager) Push(ctx *context.Context, image string, flags []string) error {
	if err := runCommand(ctx, ".", "docker", "push", image); err != nil {
		return fmt.Errorf("failed to push %s: %w", image, err)
	}
	return nil
}

This would also force fixing the imager interface accordingly.

[caarlos0]

hmmm that does not seem too bad @gal-legit... 100% doable, I think!

[ethan-lowman-dd]

Good catch, I missed that docker push has the digest in the output. I wonder whether that's an API contract though, or if multiple digests could ever show up in the output?

docker manifest push has the digest as the only output, so that's probably more likely to be an API contract of the command.

docker buildx build ... --push has the digest in the output, thought the output looks like it's only intended to be read by a human. However, I don't see that goreleaser uses this build & push command.

[gal-legit]

@ethan-lowman-dd

I wonder whether that's an API contract though

I failed to find documentation about it for the docker client, yet IMO it's pretty safe to assume that it won't change because:

1. The docker client outputs in this format for a long time, so changing it would break compatibility for lots of projects.
2. Without getting the digest from the registry, the client doesn't really have any way to use the remote image without fear of race conditions.
3. Although the client doesn't state that, the Docker Registry HTTP API clearly states that the digest is returned from the server upon completion. So the only way to get it is from the client running the push (the client might change the output format; I addressed that in the first point). see: https://docs.docker.com/registry/spec/api/#completed-upload

, or if multiple digests could ever show up in the output?

AFAIK, the only case that it might happen is by using the --all-tags flag, but goreleaser doesn't do that.

p.s. you can use crane push -v to get a better sense of the behavior.

[ethan-lowman-dd]

I double checked -- the docker push output has been stable for 6 or 7 years: https://github.com/docker/docker-ce/blame/44a430f4c43e61c95d4e9e9fd6a0573fa113a119/components/engine/distribution/push_v2.go#L229

Seems okay to parse that output since it's so much simpler than the alternative.

For manifest pushes, the docker manifest commands are marked as experimental, but it seems like a design goal that the output of docker manifest push is just the pushed digest, so parsing that seems reasonable as well.

[ethan-lowman-dd]

I don't think #3540 fully closes this issue. We still need support for extracting the pushed manifest digest and to wire the digest into the sign pipeline around here:

goreleaser/internal/pipe/sign/sign.go

Line 156 in ee6225f

env["artifactID"] = art.ID()

[caarlos0]

ah, right

[caarlos0]

@ethan-lowman-dd #3556 and #3555 should take care of the remaining parts

[caarlos0]

should be good now

[caarlos0]

thanks everyone!