Automatically resolve Docker images to immutable digests #1104
Conversation
| @@ -207,72 +204,6 @@ func TestEmpire_Deploy_ImageNotFound(t *testing.T) { | |||
| s.AssertExpectations(t) | |||
| } | |||
|
|
|||
| func TestEmpire_Deploy_Concurrent(t *testing.T) { | |||
There was a problem hiding this comment.
I'm removing this test, mainly because it's been buggy and fails randomly (pretty sure it's the test itself, and not what it's actually testing). I'll try to follow up in the future with a better version of this test.
registry/registry.go
Outdated
| } | ||
|
|
||
| if len(i.RepoDigests) <= 0 { | ||
| return img, &DigestError{Image: img.String()} |
There was a problem hiding this comment.
Turns out RepoDigests is only populated from a docker pull in Docker 1.12+. This thread moby/moby#15508 explains the details of that.
We've been locked to 1.11.x for some time now, just for stability, but should probably consider upgrading to a later version.
Instead of returning an error here, this could just fallback to the existing behavior instead.
There was a problem hiding this comment.
I was just wondering about this - yeah, seems like falling back to existing behavior - or possibly having it be able to configure that you require digests - would be good.
registry/registry.go
Outdated
| } | ||
|
|
||
| if len(i.RepoDigests) <= 0 { | ||
| return img, &DigestError{Image: img.String()} |
There was a problem hiding this comment.
I was just wondering about this - yeah, seems like falling back to existing behavior - or possibly having it be able to configure that you require digests - would be good.
|
Also could use some docs/etc. |
The name type is responsible for interacting with images, like extracting Procfiles and resolving them to immutable identifiers.
This automatically resolves Docker images to an immutable identifier before saving the slug. This has numerous benefits: 1. For security, it prevents docker registry attack vectors (MiTM, credential exposure). 2. For reliability, it prevents issues from someone accidentally overwriting a tag.
|
Ok, pretty happy with this now. It now falls back to the older behavior if no digests are available, and prints info in the deployment stream. If the underlying Docker Daemon supports digests, you'll see: $ emp deploy remind101/acme-inc:master
...
Status: Image is up to date for remind101/acme-inc:master
Status: Resolved remind101/acme-inc:master to remind101/acme-inc@sha256:c6f77d2098bc0e32aef3102e71b51831a9083dd9356a0ccadca860596a1e9007If there are no digests: $ emp deploy remind101/acme-inc:master
...
Status: Image is up to date for remind101/acme-inc:master
Status: Image has no repository digests. Using remind101/acme-inc:master as image identifierAlso updated the relevant docs, and made procfile extraction use the resolved identifier. |
|
Should there be a way to configure Empire so it only accepts images with a digest? Not sure what the implications are in the fall back mode, or if it exposes us to any potential security vulnerabilities. |
That'd probably be good. I agree the fallback mode is a little fishy, since it could potentially be used as an attack vector. For the fallback, I think either a flag to enforce digests, or change the logic to use the Docker API version (or a combination of the both). |
Previously, if a digest was provided, it wouldn't be passed down to PullImage, which would end up pulling all images (the behavior of the Docker daemon).
This adds a new --docker.digests flag (DOCKER_DIGESTS env var) that allows the behavior of image resolution to be controlled. The default is to use a digest if available, but can be changed to enforce digests (error out if the image has no immutable digest), or disabled to get the same behavior as before (no digest resolution).
|
Alrighty, updated this with a I'll run this in our staging env for a couple days and merge if no issues come up. |
|
This is working well in staging. Gonna merge. |
Closes #247
With this change, when you deploy an image using a mutable tag (e.g.
emp deploy remind101/acme-inc:latest), Empire will resolve the image reference to an immutable reference (e.g.remind101/acme-inc@sha256:c6f77d2098bc0e32aef3102e71b51831a9083dd9356a0ccadca860596a1e9007) before saving the slug. This has a number of benefits, both for security and for stability.This was always the intention, but couldn't be done until ECS supported digests (which it now does).
In this PR, the first commit is a small refactor to change the
ProcfileExtractorinterface to anImageRegistryinterface. And then the second commit is a small change that implements digest resolution.